Skip to content

Add ability to rename grouped traces #1919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Aug 15, 2017
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 41 additions & 7 deletions src/components/legend/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var anchorUtils = require('./anchor_utils');

var SHOWISOLATETIP = true;
var DBLCLICKDELAY = interactConstants.DBLCLICKDELAY;
var BLANK_STRING_REGEX = /^[\s\r]*$/;

module.exports = function draw(gd) {
var fullLayout = gd._fullLayout;
Expand Down Expand Up @@ -392,24 +393,57 @@ function drawTexts(g, gd) {
this.text(text)
.call(textLayout);

var origText = text;

if(!this.text()) text = ' \u0020\u0020 ';

var fullInput = legendItem.trace._fullInput || {},
astr;
var i, transforms, direction;
var fullInput = legendItem.trace._fullInput || {};
var needsRedraw = false;
var update = {};

// N.B. this block isn't super clean,
// is unfortunately untested at the moment,
// and only works for for 'ohlc' and 'candlestick',
// but should be generalized for other one-to-many transforms
if(['ohlc', 'candlestick'].indexOf(fullInput.type) !== -1) {
var transforms = legendItem.trace.transforms,
direction = transforms[transforms.length - 1].direction;
transforms = legendItem.trace.transforms;
direction = transforms[transforms.length - 1].direction;

update[direction + '.name'] = text;
} else if(fullInput.transforms) {
for(i = fullInput.transforms.length - 1; i >= 0; i--) {
if(fullInput.transforms[i].type === 'groupby') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to add a lib function similar to Registry.traceIs for transforms e.g. Registry,hasTransform('groupby')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Regsitry.hasTransform and Registry.getTransformIndices to accomplish this. It's less efficient than the above code, but so marginally that I think it's fine. ✅

break;
}
}

var carr = Lib.keyedContainer(fullInput, 'transforms[' + i + '].groupnames', 'group', 'name');

if(BLANK_STRING_REGEX.test(origText)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, there is a use case for a blank name - maybe less so for groupby than for regular traces, but sometimes you want the title for one trace to stand in for a few traces below it. You could argue then that this should just be testing for an actual empty string '' but a space ' ' should be allowed as the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I could just check === '' instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carr.remove(legendItem.trace._group);
needsRedraw = true;
} else {
carr.set(legendItem.trace._group, text);
}

update = carr.constructUpdate();
} else {
update.name = text;
}

var p = Plotly.restyle(gd, update, traceIndex);

astr = direction + '.name';
// If a groupby label is deleted, it seems like we need another redraw in order
// to restore the label. Otherwise it simply sets this property and the blank
// string is retained.
if(needsRedraw) {
p = p.then(function() {
return Plotly.redraw(gd);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. We shouldn't have to do this. We should fix _restyle so that it leads to the correct update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'll try to make that happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble reproducing that this was ever necessary. It seems to update just fine. Giving this the 🔪 and I think it's fine.

});
}
else astr = 'name';

Plotly.restyle(gd, astr, text, traceIndex);
return p;
});
}
else text.call(textLayout);
Expand Down
1 change: 1 addition & 0 deletions src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var BADNUM = numConstants.BADNUM;
var lib = module.exports = {};

lib.nestedProperty = require('./nested_property');
lib.keyedContainer = require('./keyed_container');
lib.isPlainObject = require('./is_plain_object');
lib.isArray = require('./is_array');
lib.mod = require('./mod');
Expand Down
116 changes: 116 additions & 0 deletions src/lib/keyed_container.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/**
* Copyright 2012-2017, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

'use strict';

var nestedProperty = require('./nested_property');

// bitmask for deciding what's updated:
var NONE = 0;
var NAME = 1;
var VALUE = 2;
var BOTH = 3;

module.exports = function keyedContainer(baseObj, path, keyName, valueName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I like it.

@rreusser can you of other situations where this could be useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It's also used in group styles, for one, though that case might be slightly more complicated. Also frames, though again, slightly more complicated. It's more a matter of realizing that unless we allow custom keys in the schema, abstracting this pattern seems like the only sane way forward.


keyName = keyName || 'name';
valueName = valueName || 'value';
var i, arr;
var changeTypes = {};

if(path && path.length) { arr = nestedProperty(baseObj, path).get();
} else {
arr = baseObj;
}

path = path || '';
arr = arr || [];

// Construct an index:
var indexLookup = {};
for(i = 0; i < arr.length; i++) {
indexLookup[arr[i][keyName]] = i;
}

var obj = {
// NB: this does not actually modify the baseObj
set: function(name, value) {
var changeType = NONE;
var idx = indexLookup[name];
if(idx === undefined) {
changeType = BOTH;
idx = arr.length;
indexLookup[name] = idx;
} else if(value !== arr[idx][valueName]) {
changeType = VALUE;
}
var newValue = {};
newValue[keyName] = name;
newValue[valueName] = value;
arr[idx] = newValue;

changeTypes[idx] = changeTypes[idx] | changeType;

return obj;
},
get: function(name) {
var idx = indexLookup[name];
return idx === undefined ? undefined : arr[idx][valueName];
},
rename: function(name, newName) {
var idx = indexLookup[name];

if(idx === undefined) return obj;
changeTypes[idx] = changeTypes[idx] | NAME;

indexLookup[newName] = idx;
delete indexLookup[name];

arr[idx][keyName] = newName;

return obj;
},
remove: function(name) {
var idx = indexLookup[name];
if(idx === undefined) return obj;
for(i = idx; i < arr.length; i++) {
changeTypes[i] = changeTypes[i] | BOTH;
}
for(i = idx; i < arr.length; i++) {
indexLookup[arr[i][keyName]]--;
}
arr.splice(idx, 1);
delete(indexLookup[name]);

return obj;
},
constructUpdate: function() {
var astr, idx;
var update = {};
var changed = Object.keys(changeTypes);
for(var i = 0; i < changed.length; i++) {
idx = changed[i];
astr = path + '[' + idx + ']';
if(arr[idx]) {
if(changeTypes[idx] & NAME) {
update[astr + '.' + keyName] = arr[idx][keyName];
}
if(changeTypes[idx] & VALUE) {
update[astr + '.' + valueName] = arr[idx][valueName];
}
} else {
update[astr] = null;
}
}

return update;
}
};

return obj;
};
17 changes: 11 additions & 6 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) {

for(i = 0; i < dataIn.length; i++) {
trace = dataIn[i];
fullTrace = plots.supplyTraceDefaults(trace, cnt, fullLayout, i);
fullTrace = plots.supplyTraceDefaults(trace, cnt, fullLayout, i, dataIn.length);
Copy link
Contributor

@etpinard etpinard Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer stashing dataIn.length on fullLayout then passing it around like this.

Something like fullLayout._dataInLength that could be set here side-by-side to fullLayout._dataLength.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually layout._dataLength is the length of the input data that I'm looking for. I think it's best not to make the count depend upon visibility (legend shouldn't fundamentally change when you toggle visibility, right?), so I think it's best to just use layout._dataLength for this.


fullTrace.index = i;
fullTrace._input = trace;
Expand All @@ -809,7 +809,12 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) {

for(var j = 0; j < expandedTraces.length; j++) {
var expandedTrace = expandedTraces[j];
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i);
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i, dataIn.length);

// The group key gets cleared. If set, pass it forward
if(expandedTrace._group) {
fullExpandedTrace._group = expandedTrace._group;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That's too bad. Do we really need this _group, i.e. could just recompute it during Legend.draw?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we could call relinkPrivateKeys here, to make this solution generalizable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I didn't like it. I'll try to clean it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to relinkPrivateKeys(fullExpandedTrace, expandedTrace), which seems to me like probably the most general approach here as long as it's not too expensive.

}

// mutate uid here using parent uid and expanded index
// to promote consistency between update calls
Expand Down Expand Up @@ -940,7 +945,7 @@ plots.supplyFrameDefaults = function(frameIn) {
return frameOut;
};

plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInIndex) {
plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInIndex, inputTraceCount) {
var traceOut = {},
defaultColor = Color.defaults[traceOutIndex % Color.defaults.length];

Expand Down Expand Up @@ -1013,13 +1018,13 @@ plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInInde
traceOut.visible = !!traceOut.visible;
}

plots.supplyTransformDefaults(traceIn, traceOut, layout);
plots.supplyTransformDefaults(traceIn, traceOut, layout, inputTraceCount);
}

return traceOut;
};

plots.supplyTransformDefaults = function(traceIn, traceOut, layout) {
plots.supplyTransformDefaults = function(traceIn, traceOut, layout, inputTraceCount) {
var globalTransforms = layout._globalTransforms || [];
var transformModules = layout._transformModules || [];

Expand All @@ -1038,7 +1043,7 @@ plots.supplyTransformDefaults = function(traceIn, traceOut, layout) {
if(!_module) Lib.warn('Unrecognized transform type ' + type + '.');

if(_module && _module.supplyDefaults) {
transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn);
transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn, inputTraceCount);
transformOut.type = type;
transformOut._module = _module;

Expand Down
69 changes: 66 additions & 3 deletions src/transforms/groupby.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,35 @@ exports.attributes = {
'with `x` [1, 3] and one trace with `x` [2, 4].'
].join(' ')
},
nameformat: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bringing in @alexcjohnson @cldougl @chriddyp @cpsievert @monfera what do you think of this syntax?

A lot of existing attributes could benefit form special formatting character like this one. For example,

trace = {
  y: [1, 2, 1]
  marker: {
    size: [20, 30, 10]
  },
  hovertext: '%marker.size'
}

would be a nice shortcut to show each point's marker.size value on hover.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I like %. Though maybe I'd vote for enforcing it with {} e.g. %{name} similar to ES6 ${name}.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and I would make user spell out %{name} and %{group} as opposed to e.g. %t and %g

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the %{group} format.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is cool! I hadn't thought about extending this to other keys but I can imagine that going off in all sorts of directions in the future (%{marker.size*5}???).

%{...} seems sufficiently useless on its own that we can do this without calling it a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Like a general substitution framework? Yeah, the goal here was just a one-off custom substitution in which it only knows about a couple properties. But definitely possibilities. The only thing then would be choosing awkward names like %{group} and %{name} (instead of %{trace}) so that it's backward compatibility in case we substitute trace properties instead of special/custom/ad hoc keys.

Copy link
Contributor Author

@rreusser rreusser Aug 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etpinard @alexcjohnson I've added Lib.templateString that performs these substitutions. At the moment it just grabs object properties if it's simple or uses nestedProperty if the key is more complicated than just a plain word. For this case the only data available is %{trace} and %{group}, but it at least opens the door to a unified interface for taking this further (or worst-case-scenario it's only a couple lines of code if it's not more generally useful). Feedback welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See:

lib.templateString = function(string, obj) {

valType: 'string',
description: [
'Pattern by which grouped traces are named. If only one trace is present,',
'defaults to the group name (`"%g"`), otherwise defaults to the group name',
'with trace name (`"%g (%t)"`). Available escape sequences are `%g`, which',
'inserts the group name, and `%t`, which inserts the trace name. If grouping',
'GDP data by country when more than one trace is present, for example, the',
'default "%g (%t)" would return "Monaco (GDP per capita)".'
].join(' ')
},
groupnames: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I missed this during my first review. I'm not a fan of groupnames being an array container.

We could instead declare groupnames as a data_array and expecting its length to be equal to groups.length, but that would lead to some (maybe a lot of) duplication.

Maybe we could groupname attribute to the styles container below instead which already expects a target group?

Actually, do we even need a groupnames attribute? groups items can be any strings, so setting groupnames[i].name is equivalent to changing the corresponding groups item.

I apologize for not catching this sooner.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting groupnames[i].name is equivalent to changing the corresponding groups item.

Not quite - the name will be set by nameformat - ie a combination of the group value and the trace name by default - but when you want to override that (perhaps by typing something specific in the editable legend) you'll need this.

We could instead declare groupnames as a data_array and expecting its length to be equal to groups.length, but that would lead to some (maybe a lot of) duplication.

Or ambiguity, if the names were not perfectly duplicated. What would that mean?

Maybe we could groupname attribute to the styles container below instead which already expects a target group?

That sounds great! so each entry in styles would have target and optionally value and/or name? No need to have two separate containers both keyed on the group value.

Copy link
Contributor Author

@rreusser rreusser Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem is that groups have no correspondence with groupnames, which I think rules out a plain array for the API. I think there's a good argument to be made though for merging styles with groupnames. Actually in hindsight, that does seem best to me. Not sure why I didn't realize that sooner…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds great! so each entry in styles would have target and optionally value and/or name?

Exactly. I'd vote for name over value, but I don't have a strong opinion here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value is all the other styles... actually, would it work to just put name inside value, and not have to create any new API? We'd still of course have to point the editing routine at styles[i].value.name but seems like that's all, right?

Copy link
Contributor Author

@rreusser rreusser Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's the best option. 👍 Then it really is just a style property (which could be set anyway and in fact we'd be doing something surprising not to use what's already possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, IMO, TBH, I'm even happier now that I took the time to implement keyedContainer…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it work to just put name inside value

good call 👍

_isLinkedToArray: 'groupname',
group: {
valType: 'string',
role: 'info',
description: [
'An group to which this name applies. If a `group` and `name` are specified,',
'that name overrides the `nameformat` for that group\'s trace.'
].join(' ')
},
name: {
valType: 'string',
role: 'info',
description: [
'Trace names assigned to the grouped traces of the corresponding `group`.'
].join(' ')
}
},
styles: {
_isLinkedToArray: 'style',
target: {
Expand Down Expand Up @@ -71,7 +100,8 @@ exports.attributes = {
* @return {object} transformOut
* copy of transformIn that contains attribute defaults
*/
exports.supplyDefaults = function(transformIn) {
exports.supplyDefaults = function(transformIn, traceOut, layout, traceIn, inputTraceCount) {
var i;
var transformOut = {};

function coerce(attr, dflt) {
Expand All @@ -83,12 +113,24 @@ exports.supplyDefaults = function(transformIn) {
if(!enabled) return transformOut;

coerce('groups');
coerce('nameformat', inputTraceCount > 1 ? '%g (%t)' : '%g');

var nameFormatIn = transformIn.groupnames;
var nameFormatOut = transformOut.groupnames = [];

if(nameFormatIn) {
for(i = 0; i < nameFormatIn.length; i++) {
nameFormatOut[i] = {};
Lib.coerce(nameFormatIn[i], nameFormatOut[i], exports.attributes.groupnames, 'group');
Lib.coerce(nameFormatIn[i], nameFormatOut[i], exports.attributes.groupnames, 'name');
}
}

var styleIn = transformIn.styles;
var styleOut = transformOut.styles = [];

if(styleIn) {
for(var i = 0; i < styleIn.length; i++) {
for(i = 0; i < styleIn.length; i++) {
styleOut[i] = {};
Lib.coerce(styleIn[i], styleOut[i], exports.attributes.styles, 'target');
Lib.coerce(styleIn[i], styleOut[i], exports.attributes.styles, 'value');
Expand Down Expand Up @@ -130,9 +172,15 @@ exports.transform = function(data, state) {
return newData;
};

function computeName(pattern, traceName, groupName) {
return pattern.replace(/%g/g, groupName)
.replace(/%t/g, traceName);
}


function transformOne(trace, state) {
var i, j, k, attr, srcArray, groupName, newTrace, transforms, arrayLookup;
var groupNameObj;

var opts = state.transform;
var groups = trace.transforms[state.transformIndex].groups;
Expand All @@ -153,6 +201,10 @@ function transformOne(trace, state) {
styleLookup[styles[i].target] = styles[i].value;
}

if(opts.groupnames) {
groupNameObj = Lib.keyedContainer(opts, 'groupnames', 'group', 'name');
}

// An index to map group name --> expanded trace index
var indexLookup = {};

Expand All @@ -162,7 +214,18 @@ function transformOne(trace, state) {

// Start with a deep extend that just copies array references.
newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace);
newTrace.name = groupName;
newTrace._group = groupName;

var suppliedName = null;
if(groupNameObj) {
suppliedName = groupNameObj.get(groupName);
}

if(suppliedName) {
newTrace.name = suppliedName;
} else {
newTrace.name = computeName(opts.nameformat, trace.name, groupName);
}

// In order for groups to apply correctly to other transform data (e.g.
// a filter transform), we have to break the connection and clone the
Expand Down
Loading