Transform react #2577

alexcjohnson · 2018-04-22T03:47:51Z

A collection of fixes for transforms, particularly in conjunction with Plotly.react - closing #2470 and #2508, and along the way completing #1410.

The primary issue with transforms and react was the remapping of transformed arrays that we do at the end of supplyDefaults - the signal not to do this before a recalc, as initiated by Plotly.restyle/relayout/update was that we had already deleted gd.calcdata; but in Plotly.react we call supplyDefaults before we know what kind of a change we have, so we haven't deleted calcdata yet. The solution was pretty simple, to conditionally break out the remapping step and apply it later as required. This is fcc459d

The other major thing I did here was to standardize the meaning of trace._length. It can either be a positive integer or null, and every trace type MUST define this during supplyDefaults:

If it's an integer, that means the trace data is 1D and all the arrayAttrs describe the same collection of objects, point-by-point.
- Transforms are allowed, and they will operate on the first _length points in each array.
If it's null, that means either it has 2D data, or it's 1D but the arrays have different meanings - like nodes and links in sankey, or vertices and vertex indices in mesh3d.
- Transforms are disabled. If we ever develop a transform that makes sense for any of these cases, we'll have to revise this condition, but currently all our transforms assume 1D data.

cc @etpinard @nicolaskruchten
cc @bpostlethwaite @VeraZab this may require some minor updates to the transforms in chart studio for robustness, though I suspect they'll mostly work already... one thing to note is I had the built-in transforms here update _length when they run, but I don't think that's strictly necessary, since the regular supplyDefaults runs after the transform anyway.

@VeraZab

thanks @VeraZab!

so aggregations and other common routines only have to look at _length

…forms also makes transforms work with parcoords (and by extension splom) by having findArrayAttributes dig into container arrays

plus whatever Array.sort is of course... O(n log(n)) or whatever

heatmap and carpet used opposite meanings here...

…forms if it's falsy

nicolaskruchten · 2018-04-22T17:59:17Z

Exciting! So how can a UI like react-chart-editor determine if a given trace in a figure is elligible for transforms? Is this available in the schema or in _fullData or somewhere or should we inline a list of trace types that support transforms?

alexcjohnson · 2018-04-22T22:59:07Z

So how can a UI like react-chart-editor determine if a given trace in a figure is eligible for transforms?

If you have _fullData look at trace._length: if it's a positive integer you can transform it, if it's null you can't. One caveat though, if the trace isn't visible it does not need to define _length so you wouldn't know. I suppose that applies everything though so it must be standard for the editor to not be able to really edit invisible traces.

We could add this to the schema I guess, but there are a few cases where the type isn't enough to know whether you can transform it or not. Chiefly heatmap and contour, where if z is 1D you can transform it, but if it's 2D you can't.

etpinard · 2018-04-23T14:57:58Z

src/traces/box/defaults.js

+        else {
+            coerce('x0');
+            len = y.length;
+        }
    } else if(x && x.length) {


This could be hasX.

etpinard · 2018-04-23T15:01:42Z

src/traces/heatmap/convert_column_xyz.js

-        col2Calendar = trace[var2Name + 'calendar'];
+    var colLen = trace._length;
+    var col1 = trace[var1Name].slice(0, colLen);
+    var col2 = trace[var2Name].slice(0, colLen);


Oh so nice to .slice() only once!

etpinard · 2018-04-23T15:04:33Z

src/traces/histogram/defaults.js

-        y = coerce('y');
+    var x = coerce('x');
+    var y = coerce('y');
+    var hasX = x && x.length;


I suspect x && x.length is faster than Array.isArray(x) && x.length. Might be worth standardizing lib/is_array.js (as e.g. Lib.isNonEmptyArray) ?

e.g. Lib.isNonEmptyArray

Ah hmm, before I do that, I want to check this against our previous behavior for trace types that don't necessarily need both coordinates specified - like scatter, which can use x0/dx along with a y array. For example if x=[], y=[1, 2, 3] we should treat that as _length=0, not 3, right? I'll need to look over our test coverage for those cases...

When I looked at empty-array behavior in more detail 6fae229 some of these went away. So I've not created Lib.isNonEmptyArray for the time being.

etpinard · 2018-04-23T15:11:05Z

src/traces/mesh3d/defaults.js

@@ -86,4 +86,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
    }

    coerce('text');
+
+    // disable 1D transforms
+    traceOut._length = null;


Actually, mesh3d only expects 1D data, so _length could have a meaning and transforms could work.

Ah nevermind, i/j/k and x/y/z lengths don't match for mesh3d traces. Can you add a comment about this above this line like you did for sankey traces?

etpinard · 2018-04-23T15:22:36Z

src/transforms/sort.js

-            }
-        }
+    for(i = 0; i < len; i++) {
+        indices[i] = sortedArray[i].i;


Brilliant ✨

etpinard · 2018-04-23T15:24:35Z

src/lib/nested_property.js

 */
-var INFO_PATTERNS = /(^|\.)((domain|range)(\.[xy])?|args|parallels)$/;


TODO:

check that the old toolpanel does not break

Moreover @alexcjohnson, what if in the very unlikely scenario of someone writing in that their app relies on the old pruning behavior, what should we do?

I suspect reverting this commit would break some test(s) added in this PR (which one by the way?), so maybe we could formulate a solution for that hypothetical user?

The reason I made this commit now was the trace._length = null, which in some cases was getting pruned later on in supplyDefaults by attributes with dflt: null and breaking my redraw all the things test addition. I could have handled this in a few different ways:

use something other than null for _length - I considered false but to me that had the implication of "has no length" rather than what I wanted, "(1D) length does not apply." Anyway the whole idea that we're pruning in the middle of supplyDefaults which is really only supposed to be building the _full* objects I wanted to avoid.

make a special code path that does no pruning or unsetting - like nestedProperty.onlySet(val) - but that's adding complexity so not ideal.

get rid of all dflt: null (and perhaps even enforce that) throughout our attributes. I still think we should do that, but I opted not to do it here.

Anyway, to the question:

what if in the very unlikely scenario of someone writing in that their app relies on the old pruning behavior, what should we do?

If it leads to a visible change in the plot, we should fix it so the API does not contain such a dependence on the existence of empty containers ;)
If it just alters the data structures (data and layout I guess, after a restyle/relayout) I can't really predict why a user would be bothered by this, but we can help them wrap their restyle/relayout with something like if(val === null) { pruneContainersPulledFromThisCommit(...) }

Great answer. Thanks!

etpinard · 2018-04-23T15:38:15Z

src/lib/is_array.js

+}
+
+function is1D(a) {
+    return !isArrayOrTypedArray(a[0]);


Maybe isArray1D would be clearer as this assumes that you're passing an array (or at least something you can index in).

We might want to add a comment above about that assumption too, in case someone in the future wants to turn this into:

return Array.isArray(a) && !isArrayOrTypedArray(a[0]);

2a41f9e -> isArray1D

etpinard · 2018-04-23T15:46:58Z

src/plot_api/plot_api.js

+        // "true" skips updating calcdata and remapping arrays from calcTransforms,
+        // which supplyDefaults usually does at the end, but we may need to NOT do
+        // if the diff (which we haven't determined yet) says we'll recalc
+        Plots.supplyDefaults(gd, true);


I'm not a big fan of this pattern.

At the very least we should turn this into Plots.supplyDefaults(gd, {skipDefaultsUpdateCalc: 1}) to make it more readable and extendable.

Personally, I'd vote for removing the newly added Plots.supplyDefaultsUpdateCalc() from Plots.supplyDefaults and call Plotly.supplyDefaultsUpdateCalc after Plots.supplyDefaults in the places that new it.

On master, we have:

where the calls in validate.js and rangeslider/draw.js (and maybe more) don't need to update calc.

Perhaps the name supplyDefaultsUpdateCalc isn't the best - though you could argue the same about calcTransform, which happens during the calc step but updates the data arrays in _fullData. supplyDefaultsUpdateCalc has one simple function when there are no transforms, to attach the new traces to the old calcdata in case we're not going to run a recalc, but when there are transforms it also (first) pulls the transformed array attributes back from the old fullTrace to the new one. So I think in fact we do need this in most instances, and there are some external callers (like streambed) that depend on this as well.

So I'm going to keep it as part of the default supplyDefaults behavior, but I'm happy to make it into an options object for clarity.

cleaner supplyDefaults API -> 2a41f9e

etpinard

Great PR.

I'll admit when you told us you were trying to fix react + transforms as part of a patch release, I was a little concerned of potential breaking changes. I'm still a little concerned about updating the pruning behavior in nestedProperty, but all in all this PR worked out pretty well I think.

etpinard · 2018-04-23T15:48:23Z

src/plots/plots.js

+        var cd0 = oldCalcdata[i][0];
+        if(cd0 && cd0.trace) {
+            if(cd0.trace._hasCalcTransform) {
+                remapTransformedArrays(cd0, newTrace);


remapTransformedArrays is only called from here, maybe we could merge it in here?

merged and 🐎 remapTransformedArrays -> 2a41f9e

etpinard · 2018-04-23T15:49:51Z

test/jasmine/tests/plot_api_test.js

+            };
+        }
+
+        function reactWith(fig) {


Very cute tests, especially this function name 💟

... and I'm impressed that bbe3533 and standardizing _length was all we needed to get transforms to work with splom and parcoords 🎉

I would actually name this reactTo personally, in both senses of the word 'to' (with and towards) :)

good call @nicolaskruchten -> reactTo in cd08479

etpinard · 2018-04-23T15:56:53Z

src/traces/heatmap/xyz_defaults.js

@@ -37,12 +39,14 @@ module.exports = function handleXYZDefaults(traceIn, traceOut, coerce, layout, x
        if(!isValidZ(z)) return 0;

        coerce('transpose');
+
+        traceOut._length = null;


What if instead of null, 2D arrays would link a length-2 array to _length of row and column lengths? Of course, this won't solve the transform-and-2d-array-traces problems by itself, but it might help us clean up some indexing downstream logic like extracting values for hover labels and event data (e.g. here and here)

What if instead of null, 2D arrays would link a length-2 array to _length of row and column lengths?

I could imagine that being helpful but it sounds like a bit of a project in itself - not quite sure how it would mesh with the logic you linked for example, which may be at least partially after reshaping from columns to a grid. Anyway we can add that later, so I'd like to defer it for now.

so I'd like to defer it for now.

Sounds good.

etpinard · 2018-04-23T16:09:15Z

src/lib/nested_property.js

 */
-var INFO_PATTERNS = /(^|\.)((domain|range)(\.[xy])?|args|parallels)$/;


Moreover @alexcjohnson, what if in the very unlikely scenario of someone writing in that their app relies on the old pruning behavior, what should we do?

I suspect reverting this commit would break some test(s) added in this PR (which one by the way?), so maybe we could formulate a solution for that hypothetical user?

etpinard · 2018-04-23T16:15:09Z

Oh, one more thing. I don't think the checklist in #2508 (comment) got completed.

- is1D -> isArray1D - improved comments - opts object instead of confusing boolean to Plots.supplyDefaults - merge & simplify remapTransformedArrays - hasX

it's not an attribute, and box/plot doesn't mind it being undefined

especially pushing auto values back to input when there's a groupby transform

alexcjohnson · 2018-04-26T18:29:25Z

Oh, one more thing. I don't think the checklist in #2508 (comment) got completed.

completed in 965bcfb

alexcjohnson · 2018-04-26T18:47:22Z

test/jasmine/tests/plot_api_test.js

+        it('tested every trace & transform type at least once', function() {
+            for(var itemType in typesTested) {
+                expect(typesTested[itemType]).toBeGreaterThan(0, itemType + ' was not tested');
+            }


So when a new trace or transform type is added we'll be forced to add it to this suite as well. (note above I excluded contourgl and, on CI, scattermapbox)

I couldn't think of a good way to ensure that every component gets tested as well... in fact looking over the list now I don't see shapes or 3D annotations, I'll have to add those. If anyone has ideas how to enforce this please chime in!

So when a new trace or transform type is added we'll be forced to add it to this suite as well

🥇

If anyone has ideas how to enforce this please chime in!

There's probably a way using Registry.componentsRegistry and the components' schema declaration (e.g. this one for range sliders). Instead of looping over traces and checking the type, we'd have to loop over gd.layout keys.

OK - still sounds a bit finicky... perhaps a later iteration we could add a test that ensures we've covered every attribute anywhere in the schema - that would be a significantly more stringent coverage test for traces and transforms as well! But if you don't mind I'll defer this for now.

But if you don't mind I'll defer this for now.

No problem 👍

nicolaskruchten · 2018-04-26T19:05:35Z

test/jasmine/tests/plot_api_test.js

+            .then(done);
+        }
+
+        mockList.forEach(function(mockSpec) {


Is there anything we can think of that could be asserted about the cartesian product here? I.e. given M1 and M2 is there anything in the JSON representation that we want to try to assert about M1.reactWith(M2).reactWith(M1) ?

There are likely some things that are desirable that are already true in all cases and might be worth locking down, and some things that are desirable that are true in all but a small subset of cases and could be fixed easily (low-hanging fruits). There are also probably a large number of desirable things that aren't true and that are hard to fix, which I'm less interested in in the short run.

I tested a few of those transitions in fcc459d#diff-5a140ce1cc87a420496039bbf8d699f1R3114 - and in fact after this PR (knock wood) I kind of don't think there will be a lot of nasty issues uncovered by testing this cartesian product. It's also going to be a very long test if you want to cover every combination, so it sounds to me more like we'd want to test them all once, offline, find the subset that fail, and incorporate just those into a new test.

etpinard · 2018-04-26T18:47:17Z

src/plots/plots.js

+ *   trace into the new one. Use skipUpdateCalc to defer this (needed by Plotly.react)
+ *
+ * gd.data, gd.layout:
+ *   are precisely what the user specified,


not quite precisely. cleanData and cleanLayout happen before.

etpinard · 2018-04-26T18:56:35Z

test/jasmine/tests/scatter_test.js

@@ -79,6 +79,22 @@ describe('Test scatter', function() {
            expect(traceOut.visible).toBe(false);
        });

+        [{letter: 'y', counter: 'x'}, {letter: 'x', counter: 'y'}].forEach(function(spec) {


nice test(s) here 😃

etpinard · 2018-04-26T18:59:00Z

src/traces/violin/plot.js

@@ -159,9 +159,6 @@ module.exports = function plot(gd, plotinfo, cd) {
                bPosPxOffset = boxLineWidth;
            }

-            // do not draw whiskers on inner boxes
-            trace.whiskerwidth = 0;


etpinard · 2018-04-26T19:00:50Z

src/components/colorscale/calc.js

+        // We pushed a colorscale back to input, which will change the default autocolorscale next time
+        // to avoid spurious redraws from Plotly.react, update resulting autocolorscale now
+        // This is a conscious decision so that changing the data later does not unexpectedly
+        // give you a new colorscale


thanks for 📚

etpinard · 2018-04-26T19:02:44Z

src/transforms/groupby.js

+
+function styleUpdater(groupName, transformIndex) {
+    return function(trace, attr, value) {
+        Lib.keyedContainer(


Ricky's dream lives on 🌈

etpinard · 2018-04-26T19:06:54Z

src/transforms/groupby.js

@@ -73,7 +73,8 @@ exports.attributes = {
                'For example, with `groups` set to *[\'a\', \'b\', \'a\', \'b\']*',
                'and `styles` set to *[{target: \'a\', value: { marker: { color: \'red\' } }}]',
                'marker points in group *\'a\'* will be drawn in red.'
-            ].join(' ')
+            ].join(' '),
+            _compareAsJSON: true


Nice solution 👏

etpinard · 2018-04-26T19:17:37Z

test/jasmine/tests/plot_api_test.js

+        it('tested every trace & transform type at least once', function() {
+            for(var itemType in typesTested) {
+                expect(typesTested[itemType]).toBeGreaterThan(0, itemType + ' was not tested');
+            }


So when a new trace or transform type is added we'll be forced to add it to this suite as well

🥇

If anyone has ideas how to enforce this please chime in!

There's probably a way using Registry.componentsRegistry and the components' schema declaration (e.g. this one for range sliders). Instead of looping over traces and checking the type, we'd have to loop over gd.layout keys.

etpinard · 2018-04-26T19:20:28Z

src/plots/attributes.js

@@ -156,5 +156,13 @@ module.exports = {
            ].join(' ')
        },
        editType: 'calc'
+    },
+    transforms: {


Wow. This means transforms will appear on https://plot.ly/javascript/reference/. I guess they are now an official feature 😬

etpinard · 2018-04-26T19:22:05Z

src/traces/contourcarpet/calc.js

@@ -99,7 +99,7 @@ function heatmappishCalc(gd, trace) {
        z: z,
    };

-    if(trace.contours.type === 'levels') {
+    if(trace.contours.type === 'levels' && trace.contours.coloring !== 'none') {


Did omitting coloring !== 'none' lead to a bug outside of Plotly.react?

possibly for the editor - zmin, zmax, and zauto were set in _fullData, implying that these were useful attributes for this case when in fact they are ignored.

etpinard · 2018-04-26T19:23:10Z

src/traces/contourcarpet/defaults.js

@@ -56,7 +56,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
        var isConstraint = (coerce('contours.type') === 'constraint');

        // Unimplemented:
-        // coerce('connectgaps', Lib.is1D(traceOut.z));
+        // coerce('connectgaps', Lib.isArray1D(traceOut.z));


Probably best to 🔪 these lines completely as well as the connectgaps declaration in ./attributes.js.

etpinard · 2018-04-26T20:34:56Z

@alexcjohnson you might need to add a longer test timeout in one of your new tests in plot_api_test.js to make them pass consistently

Apart from that, this PR is 🏅

💃 for me.

alexcjohnson · 2018-04-26T21:51:10Z

Toolpanel works fine, the only thing I had to do was remove this line:
https://github.com/plotly/streambed/blob/master/shelly/plotlyjs/static/plotlyjs/src/plotly.js#L29

As far as I can tell Plotly.Heatmap.hasColumns isn't used anywhere anyway, but if I'm missing something and it still is needed we can point it to Lib/isArray1D.

alexcjohnson added 8 commits April 21, 2018 23:26

remove no longer needed hack for finance in findArrayAttributes

bbe3533

thanks @VeraZab!

_commonLength -> _length

ed24765

so aggregations and other common routines only have to look at _length

fix #2508, fix #2470 - problems with Plotly.react and aggregate trans…

fcc459d

…forms also makes transforms work with parcoords (and by extension splom) by having findArrayAttributes dig into container arrays

standardize transforms handling of _length

7044a13

🐎 refactor sort transform from O(n^2) to O(n)

88b7b43

plus whatever Array.sort is of course... O(n log(n)) or whatever

heatmap&carpet/has_columns -> Lib.is1D

cf4c9c3

heatmap and carpet used opposite meanings here...

close #1410 - yes, stop pruning in nestedProperty

e84d4b9

ensure every trace defines _length in supplyDefaults, and abort trans…

b436d52

…forms if it's falsy

alexcjohnson added bug something broken type: maintenance labels Apr 22, 2018

etpinard reviewed Apr 23, 2018

View reviewed changes

alexcjohnson added 2 commits April 24, 2018 10:56

react+transforms PR review edits

2a41f9e

- is1D -> isArray1D - improved comments - opts object instead of confusing boolean to Plots.supplyDefaults - merge & simplify remapTransformedArrays - hasX

fixes and test for checklist in #2508

965bcfb

etpinard added this to the 1.37.0 milestone Apr 24, 2018

alexcjohnson added 11 commits April 24, 2018 18:48

some fixes and tests for empty data arrays

6fae229

rename violins mock that doesn't contain violin traces

79295f1

transforms mock

5e9aa65

clean up keyedContainer for possibly missing array

690eb95

violin should not explicitly set whiskerwidth

a244cec

it's not an attribute, and box/plot doesn't mind it being undefined

add _length and stop slicing in scattercarpet

92bd5d2

clean up handling of colorscale defaults

dc6de2f

especially pushing auto values back to input when there's a groupby transform

include count aggregates in _arrayAttrs - so they remap correctly

03956e1

in diffData I had _fullInput in the new trace, but not the old!

f439e41

_compareAsJSON for groupby styles

e47e6a9

update plot_api_test to 🔒 recent changes in react/transforms etc

aa30ad6

lint

4c70826

alexcjohnson commented Apr 26, 2018

View reviewed changes

+shapes & annotations3d in react-noop test

7279b55

nicolaskruchten reviewed Apr 26, 2018

View reviewed changes

etpinard reviewed Apr 26, 2018

View reviewed changes

alexcjohnson added 2 commits April 26, 2018 16:11

tweak docs & remove commented-out code

3e250df

reactWith -> reactTo

cd08479

separate svg and gl traces in react-noop tests

0414147

alexcjohnson merged commit 05b0ec9 into master Apr 26, 2018

alexcjohnson deleted the transform-react branch April 26, 2018 21:51

This was referenced May 2, 2018

Error when adding a new trace to a plot with Plotly.react #2602

Closed

Fix Plotly.react add/remove traces bug #2603

Merged

etpinard mentioned this pull request May 23, 2018

Fixup noci_test.sh #2656

Merged

alexcjohnson mentioned this pull request Jun 8, 2018

Support transforms that make their own data without trace input data #2714

Closed

		*/
		var INFO_PATTERNS = /(^\|\.)((domain\|range)(\.[xy])?\|args\|parallels)$/;

Transform react #2577

Transform react #2577

Conversation

alexcjohnson commented Apr 22, 2018

nicolaskruchten commented Apr 22, 2018

alexcjohnson commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Apr 23, 2018 • edited by alexcjohnson Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Apr 23, 2018

alexcjohnson commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Apr 26, 2018

alexcjohnson commented Apr 26, 2018

etpinard Apr 23, 2018 •

edited by alexcjohnson

Loading

etpinard Apr 23, 2018 •

edited

Loading