React finance precursor #2525

alexcjohnson · 2018-04-04T19:55:27Z

Various fixes to our supplyDefaults framework to make Plotly.react happier. Since this is no longer part of fixing finance charts with react per se (#2510 is going a different route now), making a separate PR for these here.

cc @etpinard

…t & table

...but we still need to make it NOT mutate the arrays it reshapes

... that I thought might fix candlestick update bugs but there's more to that

alexcjohnson · 2018-04-04T20:03:54Z

test/jasmine/tests/plot_api_test.js

+                            delete trace.zmin;
+                            delete trace.zmax;
+                        }
+                    });


Having collected all the patches we currently need to get fullJson to match up from run to run, we can then go about cleaning them up in subsequent PRs. In the meantime @nicolaskruchten @VeraZab the editor may need to be aware that these attributes are inconsistently available in the meantime 🙈

Note that I used Plotly.Plots.graphJson to generate fullJson, which already removes underscored attributes and functions.

Brilliant test ✨

@alexcjohnson only zmin and zmax?

@VeraZab zmin and zmax are actually not a problem for you, I don't think, as they only apply to contourcarpet where they're not even attributes (though they really should be... and if we make them into bon-a-fide attributes this error will go away).

It's the ones farther up that might cause headaches: tick0, dtick, and (for 3D and maybe occasionally for 2D) range. I think this is only when some other attribute indicates these attributes are auto-determined, so it still may not be an issue as you probably don't even want to display them in these cases, but I'm not sure about that.

The way to test would be to arrange a situation where you DO see those attributes, then make some totally unrelated minor change (tweak a color or something in a different panel) and go back and see if they're still where they're supposed to be.

Or just ignore it for now 😎 but if you happen to come across a bug that looks like ^^ yell at me to fix this.

alexcjohnson · 2018-04-04T20:16:23Z

src/plots/plots.js

+    // 'regular' loop - make sure container traces (eg carpet) calc before
+    // contained traces (eg contourcarpet)
+    for(i = 0; i < fullData.length; i++) calci(i, true);
+    for(i = 0; i < fullData.length; i++) calci(i, false);


Because I moved convertColumnData and setConvert for carpet traces from the supplyDefaults step to the calc step, we need to ensure carpet.calc happens before contourcarpet.calc and scattercarpet.calc. I'm not sure if we're ever going to have another situation like this where one trace essentially lives inside another, but in case we do this seemed like a reasonably generic way to handle it.

etpinard · 2018-04-05T01:01:22Z

src/plots/plots.js

        trace = fullData[i];
+        _module = trace._module;
+
+        if(!!_module.isContainer !== isContainer) return;


Ha. Nice fix. I wonder if other carpet-dependent blocks (e.g. here) could be simplified (or at least generatlized) using _module.isContainer?

Probably some such blocks could... though I don't see much to do with that carpetDependents loop. It might be nice to move it out of plots/plots and into the carpet module, but probably not in this PR.

etpinard · 2018-04-05T01:04:01Z

test/jasmine/tests/plot_api_test.js

+                            delete trace.zmin;
+                            delete trace.zmax;
+                        }
+                    });


Brilliant test ✨

etpinard · 2018-04-05T01:08:20Z

src/traces/ohlc/ohlc_defaults.js

-    if(len < high.length) traceOut.high = high.slice(0, len);
-    if(len < low.length) traceOut.low = low.slice(0, len);
-    if(len < close.length) traceOut.close = close.slice(0, len);
+    traceOut._inputLength = len;


Any reason for naming this _inputLength? Parcoords and splom use _commonLength to stash their common dimensions[i].values length - which I guess is a little different than the finance trace case here and the regular traceOut._length used in (all?) other traces.

This will be converted to _length when I de-transform these traces, but I didn't want to use that (or _commonLength) here since the post-transform traces have different lengths.

etpinard · 2018-04-05T01:11:00Z

src/traces/carpet/calc.js

-    t.xp = trace.xp = map2dArray(trace.xp, x, xa.c2p);
-    t.yp = trace.yp = map2dArray(trace.yp, y, ya.c2p);
+    t.xp = trace._xp = map2dArray(trace._xp, x, xa.c2p);
+    t.yp = trace._yp = map2dArray(trace._yp, y, ya.c2p);


Are these xp and yp values used anywhere? Here they appear to be reset during the plot step.

ha good call. Nothing fails if you 🔪 these lines, which is good because the calc step had better not generate pixel positions, the axis scaling isn't known yet at that point!
Removed in 0221510

wow, then map_2d_array became totally unused -> b0af416

etpinard · 2018-04-05T01:11:46Z

src/plots/plots.js

@@ -380,7 +380,7 @@ plots.supplyDefaults = function(gd) {
        if(_module.cleanData) _module.cleanData(newFullData);
    }

-    if(oldFullData.length === newData.length) {
+    if(oldFullData.length === newFullData.length) {


How hard would it be to 🔒 this fix?

I was going to add a Plotly.react test in transform_groupby_test for this and diffing on _fullInput, hence c2b11dc, since that's the obvious place where fullData will have a different length from data after finance is converted. But on second thought perhaps that can of worms should wait for #2508? I'll make a note in that issue if you 👍. The fact that all the existing Plotly.react and other tests pass means these didn't cause problems at least.

But on second thought perhaps that can of worms should wait for #2508?

sounds good 👌

etpinard · 2018-04-05T01:13:01Z

src/plot_api/plot_api.js

    for(i = 0; i < oldFullData.length; i++) {
-        trace = newFullData[i];
+        trace = newFullData[i]._fullInput;
+        if(seenUIDs[trace.uid]) continue;


similarly here, can we 🔒 this thing?

etpinard · 2018-04-05T01:50:03Z

src/traces/carpet/calc.js

@@ -65,10 +65,6 @@ module.exports = function calc(gd, trace) {
    // create conversion functions that depend on the data
    trace.setScale();

-    // Convert cartesian-space x/y coordinates to screen space pixel coordinates:
-    t.xp = trace._xp = map2dArray(trace._xp, x, xa.c2p);
-    t.yp = trace._yp = map2dArray(trace._yp, y, ya.c2p);


etpinard · 2018-04-05T13:13:35Z

Great PR 💃

alexcjohnson added 7 commits April 4, 2018 15:48

clean up catch/done in plot_api_test

28d073f

non-attribute modifications to _fullData should be _private, in carpe…

c802d79

…t & table

simplify convert_column_xyz API

16a22f3

...but we still need to make it NOT mutate the arrays it reshapes

have Plotly.react diff _fullInput instead of the final trace

035c98d

bug in relinkPrivateKeys condition

95911da

... that I thought might fix candlestick update bugs but there's more to that

stop overwriting fullTrace arrays in convert_column_xyz

6b201a1

stop slicing finance input arrays during supplyDefaults

5546ec7

alexcjohnson added type: maintenance labels Apr 4, 2018

alexcjohnson commented Apr 4, 2018

View reviewed changes

etpinard reviewed Apr 5, 2018

View reviewed changes

🔪 obsolete _xp, _yp from carpet

0221510

etpinard reviewed Apr 5, 2018

View reviewed changes

alexcjohnson added 2 commits April 4, 2018 21:52

even more became obsolete 🎉

b0af416

failTest in transform_groupby_test

c2b11dc

alexcjohnson mentioned this pull request Apr 5, 2018

Plotly.react sequence and transforms #2508

Closed

alexcjohnson merged commit acc7d81 into master Apr 5, 2018

alexcjohnson deleted the react-finance-precursor branch April 5, 2018 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React finance precursor #2525

React finance precursor #2525

alexcjohnson commented Apr 4, 2018

alexcjohnson Apr 4, 2018

etpinard Apr 5, 2018

VeraZab Apr 5, 2018

alexcjohnson Apr 5, 2018

alexcjohnson Apr 4, 2018

etpinard Apr 5, 2018

alexcjohnson Apr 5, 2018

etpinard Apr 5, 2018

etpinard Apr 5, 2018

alexcjohnson Apr 5, 2018

etpinard Apr 5, 2018

alexcjohnson Apr 5, 2018

alexcjohnson Apr 5, 2018

etpinard Apr 5, 2018

alexcjohnson Apr 5, 2018

etpinard Apr 5, 2018

etpinard Apr 5, 2018

etpinard Apr 5, 2018

etpinard commented Apr 5, 2018

React finance precursor #2525

React finance precursor #2525

Conversation

alexcjohnson commented Apr 4, 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 5, 2018