Rework groupby style syntax into array #1465

rreusser · 2017-03-10T16:35:51Z

As discussed, groupby transform styling permits arbitrary keys which, in addition to not fitting into the plot schema, will fail if a group ends with *src. This PR starts the switch to array-of-objects style by first simply breaking all of the tests.

rreusser · 2017-03-10T17:13:55Z

src/transforms/groupby.js

@@ -35,14 +35,24 @@ exports.attributes = {
        ].join(' ')
    },
    style: {


@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.

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

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.

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?

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.

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

Is that an acceptable change for cleanData?

rreusser · 2017-03-10T17:15:26Z

src/transforms/groupby.js

-            'marker points in group *\'a\'* will be drawn in red.'
-        ].join(' ')
+        target: {
+            valType: 'any',


Does valType: 'any' make sense or should it be strings? As-is styling by numbers is implicitly converted to a string-keyed object anyway, so I'm not 100% sure the right answer here.

Maybe weird, maybe not:

not so weird:

0.20000000000000001 === 0.2 true

We definitely want numbers treated as strings for these purposes, so target: 1 should match '1' in the data, and vice versa.

valType: 'string' might make sense to help accomplish this - with strict omitted so numbers get stringified. That would cause us to ignore array, object, null, and undefined targets, which seems right. All else equal I might have permitted booleans to pass through (stringified) but if it simplifies things - which it looks like it does - then lets leave those out.

Sounds good. I guess my discomfort is just in relying upon implicit conversion of keys to strings to sort out numerical identity. Like if your group were 0.2 and your style key were computed in JS as 0.3 - 0.1, I don't think it would find the right style.

Not really a problem in JSON, but a little weird in plain JS API usage:

I think that would still be a problem with numeric keys, unless you implement some sort of fuzzy equality, which sounds even more dangerous.

Agreed. FWIW almost-equal combines absolute and relative tolerance in a way that seems generally desirable, but I think definitely best to ignore it and just keep in mind that it's a little picky.

rreusser · 2017-03-10T17:22:44Z

src/transforms/groupby.js

+    // If the styles are an array, then translate it into an index keyed on the
+    // target. Otherwise just use the object provided for the sake of backward
+    // compatibility.
+    if(Array.isArray(style)) {


Note: I could have translated the object in to array style format to get a nice consistent result on fullData, except the useful format is actually as originally defined since it needs a lookup so that it would just have to be translated back to the key-value style down below. That way would be a bit cleaner and may need to happen; this way just minimizes the back and forth. No strong feelings here.

backward-compatibility fixes should go in cleanData so they only happen when new data comes in, and they change the data to current format.

Failed to initially appreciate the full meaning of "only when new data comes in." Moved to cleanData. 👍

rreusser · 2017-03-10T19:47:44Z

Noticed a small typo in the style docs. No rush on the review, but I'm content with this for now.

rreusser · 2017-03-10T20:24:19Z

Failure appears to be a fluke due to cholorpleth, perhaps? I'll push a dummy commit in a bit unless someone wants to hit the rebuild button.

alexcjohnson · 2017-03-10T20:26:22Z

test/jasmine/tests/transform_groupby_test.js

+                    }
+                }, {
+                    target: 'b',
+                    value: {
                        mode: 'markers+lines', // heterogeonos attributes are OK: group 'a' doesn't need to define this


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?

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.

@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.

rreusser · 2017-05-04T05:00:14Z

Trying to close out PRs. Reading through this, I'm not sure if I have anything to add to it.

etpinard · 2017-05-08T19:19:45Z

Trying to close out PRs. Reading through this, I'm not sure if I have anything to add to it.

Looks like https://github.com/plotly/plotly.js/pull/1465/files#r105486264 was never addressed.

Switch groupby tests to failing syntax

f55e58d

rreusser added the status: in progress label Mar 10, 2017

rreusser self-assigned this Mar 10, 2017

Rework groupby styles into array of objects

67058c9

rreusser added status: reviewable and removed status: in progress labels Mar 10, 2017

rreusser commented Mar 10, 2017

View reviewed changes

Fix groupby style docs

7beddad

rreusser commented Mar 10, 2017

View reviewed changes

rreusser added 5 commits March 10, 2017 13:33

Move old-style groupby styles fix to cleanData

f67198b

Switch groupby style.target to string

3cc16ba

Fix multitransform test

1543892

Add isLinkedToArray to groupby style attrs

8df0096

Fix typo in groupby style doc string

49e24ad

alexcjohnson reviewed Mar 10, 2017

View reviewed changes

rreusser mentioned this pull request Jun 16, 2017

Rework transform style into array syntax #1794

Merged

rreusser closed this Jun 16, 2017

rreusser deleted the transform-style-rework branch September 20, 2017 19:36

Uh oh!

Rework groupby style syntax into array #1465

Rework groupby style syntax into array #1465

Uh oh!

Conversation

rreusser commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rreusser Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rreusser commented Mar 10, 2017

Uh oh!

rreusser commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rreusser commented May 4, 2017

Uh oh!

etpinard commented May 8, 2017

Uh oh!

Uh oh!

rreusser commented Mar 10, 2017 •

edited

Loading

rreusser Mar 10, 2017 •

edited

Loading