Skip to content

Rework groupby style syntax into array #1465

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Rework groupby style syntax into array #1465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f55e58d
Switch groupby tests to failing syntax
rreusser Mar 10, 2017
67058c9
Rework groupby styles into array of objects
rreusser Mar 10, 2017
7beddad
Fix groupby style docs
rreusser Mar 10, 2017
f67198b
Move old-style groupby styles fix to cleanData
rreusser Mar 10, 2017
3cc16ba
Switch groupby style.target to string
rreusser Mar 10, 2017
1543892
Fix multitransform test
rreusser Mar 10, 2017
8df0096
Add isLinkedToArray to groupby style attrs
rreusser Mar 10, 2017
49e24ad
Fix typo in groupby style doc string
rreusser Mar 10, 2017
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
38 changes: 27 additions & 11 deletions src/plot_api/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ function cleanAxRef(container, attr) {
// Make a few changes to the data right away
// before it gets used for anything
exports.cleanData = function(data, existingData) {

// Enforce unique IDs
var suids = [], // seen uids --- so we can weed out incoming repeats
uids = data.concat(Array.isArray(existingData) ? existingData : [])
Expand Down Expand Up @@ -348,18 +347,35 @@ exports.cleanData = function(data, existingData) {

if(!Lib.isPlainObject(transform)) continue;

if(transform.type === 'filter') {
if(transform.filtersrc) {
transform.target = transform.filtersrc;
delete transform.filtersrc;
}
switch(transform.type) {
case 'filter':
if(transform.filtersrc) {
transform.target = transform.filtersrc;
delete transform.filtersrc;
}

if(transform.calendar) {
if(!transform.valuecalendar) {
transform.valuecalendar = transform.calendar;
if(transform.calendar) {
if(!transform.valuecalendar) {
transform.valuecalendar = transform.calendar;
}
delete transform.calendar;
}
break;

case 'groupby':
if(transform.style && !Array.isArray(transform.style)) {
var prevStyles = transform.style;
var styleKeys = Object.keys(prevStyles);

transform.style = [];
for(var j = 0; j < styleKeys.length; j++) {
transform.style.push({
target: styleKeys[j],
value: prevStyles[styleKeys[j]]
});
}
}
delete transform.calendar;
}
break;
}
}
}
Expand Down
53 changes: 40 additions & 13 deletions src/transforms/groupby.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,25 @@ exports.attributes = {
].join(' ')
},
style: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard @alexcjohnson Comparing this to similar cases, I'm not 100% sure what _isLinkedToArray does or whether it's needed here. I've omitted it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I should probably let @etpinard handle this but I think you want both _isLinkedToArray and _arrayAttrRegexps - the latter of which won't get used yet but when we extend the array editing stuff I did in relayout to restyle it will be important.

see https://github.com/plotly/plotly.js/blob/master/src/components/updatemenus/attributes.js#L17-L52

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my template, but I had a hard time tracking down exactly what it does. See: registry.js#L100-L105. It seems like since groupby has no layoutAttributes, this key will have no effect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, at the moment _arrayAttrRegexps doesn't do anything inside traces, it's just a question of if we want to put it in so we don't forget about this when we do get to implementing it. Maybe better not to include it, in case we implement it a different way. I think _isLinkedToArray matters to plot_schema already though, doesn't it?

Copy link
Contributor Author

@rreusser rreusser Mar 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Added _isLinkedToArray but did not add _arrayAttrRegexps, which I think only applies to nested array attributes like updatemenus[...].buttons. Granted this is transforms[...].styles, but I'm not totally sure where this would need to be added. At the transform level vs. inside styles, perhaps? Glad to add it blindly, but without really knowing how to test it, I think maybe better to omit for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should name this attributes styles instead? All (I think) isLinkedToArray attribute have plural names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that an acceptable change for cleanData?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes 👍

valType: 'any',
dflt: {},
description: [
'Sets each group style.',
'For example, with `groups` set to *[\'a\', \'b\', \'a\', \'b\']*',
'and `style` set to *{ a: { marker: { color: \'red\' } }}',
'marker points in group *\'a\'* will be drawn in red.'
].join(' ')
_isLinkedToArray: 'style',
target: {
valType: 'string',
role: 'info',
description: [
'The group value which receives these styles.'
].join(' ')
},
value: {
valType: 'any',
role: 'info',
dflt: {},
description: [
'Sets each group style.',
'For example, with `groups` set to *[\'a\', \'b\', \'a\', \'b\']*',
'and `style` set to *[{target: \'a\', value: { marker: { color: \'red\' } }}]',
'marker points in group *\'a\'* will be drawn in red.'
].join(' ')
},
}
};

Expand Down Expand Up @@ -71,11 +82,22 @@ exports.supplyDefaults = function(transformIn) {
if(!enabled) return transformOut;

coerce('groups');
coerce('style');

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

if(styleIn) {
for(var i = 0; i < styleIn.length; i++) {
styleOut[i] = {};
Lib.coerce(styleIn[i], styleOut[i], exports.attributes.style, 'target');
Lib.coerce(styleIn[i], styleOut[i], exports.attributes.style, 'value');
}
}

return transformOut;
};


/**
* Apply transform !!!
*
Expand Down Expand Up @@ -115,6 +137,7 @@ function pasteArray(newTrace, trace, j, a) {
}

function transformOne(trace, state) {
var i;
var opts = state.transform;
var groups = trace.transforms[state.transformIndex].groups;

Expand All @@ -128,9 +151,13 @@ function transformOne(trace, state) {

var arrayAttrs = PlotSchema.findArrayAttributes(trace);

var style = opts.style || {};
var style = opts.style || [];
var styleLookup = {};
for(i = 0; i < style.length; i++) {
styleLookup[style[i].target] = style[i].value;
}

for(var i = 0; i < groupNames.length; i++) {
for(i = 0; i < groupNames.length; i++) {
var groupName = groupNames[i];

var newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace);
Expand All @@ -145,9 +172,9 @@ function transformOne(trace, state) {

newTrace.name = groupName;

// there's no need to coerce style[groupName] here
// there's no need to coerce styleLookup[groupName] here
// as another round of supplyDefaults is done on the transformed traces
newTrace = Lib.extendDeepNoArrays(newTrace, style[groupName] || {});
newTrace = Lib.extendDeepNoArrays(newTrace, styleLookup[groupName] || {});
}

return newData;
Expand Down
119 changes: 100 additions & 19 deletions test/jasmine/tests/transform_groupby_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ var destroyGraphDiv = require('../assets/destroy_graph_div');
var assertDims = require('../assets/assert_dims');
var assertStyle = require('../assets/assert_style');


describe('groupby', function() {

describe('one-to-many transforms:', function() {
Expand All @@ -19,7 +18,10 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
style: [
{target: 'a', value: {marker: {color: 'red'}}},
{target: 'b', value: {marker: {color: 'blue'}}}
]
}]
}];

Expand All @@ -30,7 +32,10 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: ['b', 'a', 'b', 'b', 'b', 'a', 'a'],
style: { a: {marker: {color: 'green'}}, b: {marker: {color: 'black'}} }
style: [
{target: 'a', value: {marker: {color: 'green'}}},
{target: 'b', value: {marker: {color: 'black'}}}
]
}]
}];

Expand Down Expand Up @@ -58,6 +63,58 @@ describe('groupby', function() {
});
});

it('Accepts deprecated object notation for styles', function(done) {
var oldStyleMockData = [{
mode: 'markers',
x: [1, -1, -2, 0, 1, 2, 3],
y: [1, 2, 3, 1, 2, 3, 1],
transforms: [{
type: 'groupby',
groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
style: {
a: {marker: {color: 'red'}},
b: {marker: {color: 'blue'}}
}
}]
}];
var data = Lib.extendDeep([], oldStyleMockData);
data[0].marker = { size: 20 };

var gd = createGraphDiv();
var dims = [4, 3];

Plotly.plot(gd, data).then(function() {
assertStyle(dims,
['rgb(255, 0, 0)', 'rgb(0, 0, 255)'],
[1, 1]
);

return Plotly.restyle(gd, 'marker.opacity', 0.4);
}).then(function() {
assertStyle(dims,
['rgb(255, 0, 0)', 'rgb(0, 0, 255)'],
[0.4, 0.4]
);

expect(gd._fullData[0].marker.opacity).toEqual(0.4);
expect(gd._fullData[1].marker.opacity).toEqual(0.4);

return Plotly.restyle(gd, 'marker.opacity', 1);
}).then(function() {
assertStyle(dims,
['rgb(255, 0, 0)', 'rgb(0, 0, 255)'],
[1, 1]
);

expect(gd._fullData[0].marker.opacity).toEqual(1);
expect(gd._fullData[1].marker.opacity).toEqual(1);
}).then(done);

// The final test for restyle updates using deprecated syntax
// is ommitted since old style syntax is *only* sanitized on
// initial plot, *not* on restyle.
});

it('Plotly.restyle should work', function(done) {
var data = Lib.extendDeep([], mockData0);
data[0].marker = { size: 20 };
Expand Down Expand Up @@ -92,7 +149,10 @@ describe('groupby', function() {
expect(gd._fullData[1].marker.opacity).toEqual(1);

return Plotly.restyle(gd, {
'transforms[0].style': { a: {marker: {color: 'green'}}, b: {marker: {color: 'red'}} },
'transforms[0].style': [[
{target: 'a', value: {marker: {color: 'green'}}},
{target: 'b', value: {marker: {color: 'red'}}}
]],
'marker.opacity': 0.4
});
}).then(function() {
Expand Down Expand Up @@ -192,7 +252,10 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
style: [
{target: 'a', value: {marker: {color: 'red'}}},
{target: 'b', value: {marker: {color: 'blue'}}}
]
}]
}];

Expand Down Expand Up @@ -387,7 +450,10 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
style: [
{target: 'a', value: {marker: {color: 'red'}}},
{target: 'b', value: {marker: {color: 'blue'}}}
]
}]
}];

Expand All @@ -401,17 +467,20 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
style: {
a: {
style: [{
target: 'a',
value: {
marker: {
color: 'orange',
size: 20,
line: {
color: 'red'
}
}
},
b: {
}
}, {
target: 'b',
value: {
mode: 'markers+lines', // heterogeonos attributes are OK: group 'a' doesn't need to define this
Copy link
Collaborator

Choose a reason for hiding this comment

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

not that this is new, but heterogeneous

But that makes me wonder: you don't need to supply style for every group, do you? If you omit it entirely for a group, you just get the styling contained in the parent trace, yes? Want to test that?

This has an interesting use case, I think, that wouldn't have worked (at least not nearly as well) the old way: you could have a "highlight" style to do something special to one group you're focusing on, and to focus on a different group you just change target. And if you want to focus on none, change target to some value that's not a group at all (does this work or does it complain?)

Also, there's a case we couldn't have previously: what if two targets are the same? I could imagine wanting to do this in exactly the "focus style" example... you have styles for each group (say, different marker colors) but then there's a different style you want to move between groups... in that case the actual style to apply to a given group would be the composition of all styles targeting the group. Should we support this? Or do something else if you get the same target more than once?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you omit it entirely for a group, you just get the styling contained in the parent trace, yes? Want to test that?

That should be already part of the suite. But yes, we should test that.

in that case the actual style to apply to a given group would be the composition of all styles targeting the group. Should we support this? Or do something else if you get the same target more than once?

I don't see a problem with supporting multiple styles item with the same target. Style items with the same target would simply be applied in sequence to the transformed traces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson No, you don't need to specify for all. Or to be more precise, you don't need to now if you didn't before. 😄

At the moment, I'm internally transforming the style array into a key-value index that matches the old specification format. So if two are the same, the last in the array will overwrite other instances. I think merging would be an appropriate use case mergeDeepNoArrays. I don't personally find it a particularly compelling use-case, but it's a pretty straightforward change to add support for pretty weird user input.

marker: {
color: 'cyan',
Expand All @@ -426,7 +495,7 @@ describe('groupby', function() {
color: 'purple'
}
}
}
}]
}]
}];

Expand All @@ -447,11 +516,14 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
style: {
a: {marker: {size: 30}},
style: [{
target: 'a',
value: {marker: {size: 30}}
}, {
// override general color:
b: {marker: {size: 15, line: {color: 'yellow'}}, line: {color: 'purple'}}
}
target: 'b',
value: {marker: {size: 15, line: {color: 'yellow'}}, line: {color: 'purple'}}
}]
}]
}];

Expand All @@ -464,7 +536,7 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
style: {/* can be empty, or of partial group id coverage */}
style: [/* can be empty, or of partial group id coverage */]
}]
}];

Expand Down Expand Up @@ -548,7 +620,10 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
// groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'],
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
style: [
{target: 'a', value: {marker: {color: 'red'}}},
{target: 'b', value: {marker: {color: 'blue'}}}
]
}]
}];

Expand All @@ -561,7 +636,10 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: [],
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
style: [
{target: 'a', value: {marker: {color: 'red'}}},
{target: 'b', value: {marker: {color: 'blue'}}}
]
}]
}];

Expand All @@ -574,7 +652,10 @@ describe('groupby', function() {
transforms: [{
type: 'groupby',
groups: null,
style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} }
style: [
{target: 'a', value: {marker: {color: 'red'}}},
{target: 'b', value: {marker: {color: 'blue'}}}
]
}]
}];

Expand Down
Loading