Per-trace axis extremes #2849

etpinard · 2018-07-26T17:16:33Z

to be merged in #2837

By trying to address @alexcjohnson 's #2837 (comment), I opened up a pretty big can of worms. There are twice as many commits here than in #2837 😱

Oh well, I think this is for the best. In brief,

67db333 adds a test that fails in Faster trace visibility toggling #2837
Axes.expand is replaced by Axes.findExtremes. To note, we no longer mutate _min and _max array into the axis to expand, but instead we fill in _extremes: {x: {min: [], max: [], y: {min: [], max: []}} objects in the traces and layout components that can expand the axis range. That extremes object is slightly cumbersome, I'm open to suggestions to make it smoother to work with.
Axes.getAutoRange now must look for the _extremes objects -- skipping the visible:false items -- corresponding to a given axis and then computes the auto range
4c250c3 is very important, setPositions was skipped previously during 'plot' edits, we must now call it to make sure bar and box (and OHLC?) can restyle visible correctly. I'll need to double-check a for more cases.
one polar mock is currently failing, but I think it is more correct now. Update: the baseline is correct, the old concatExtremes didn't pick the right values, fixed in 66df51e
~~We should now be able to change annotations and shapes visible from an edit type 'calc' to 'plot' 🐎 done in -> 9a78013~~ reverted in 5cfee13 will be for later.

- a Axes.expend clone that does not append things to ax._min/ax._max, but instead returns two arrays, a min array and max array of potential data extremes

- to be filled with the results of findExtremes

- pass gd, to look for _extremes in gd._fullData and eventually in layout container to can expand the axis ranges

- N.B. sploms are linked to multiple axes per traces, so we can't rely on single 'x' and 'y' in _extremes -> use ax._id instead!

- here we append the min/max arrays of the corresponding trace

- questionable commit, especially the part in autorange.js, but this doesn't make any test fail ?!?

- N.B. polar is the only subplot apart from cartesian that used Axes.expand and friends.

etpinard · 2018-07-26T18:04:22Z

src/plots/cartesian/autorange.js

@@ -174,15 +178,40 @@ function makePadFn(ax) {
    return function getPad(pt) { return pt.pad + (pt.extrapad ? extrappad : 0); };
 }

-function doAutoRange(ax) {
+function concatExtremes(gd, ax, k) {


Hmm. Come to think of it, this thing will be a drag. Looping over all traces, annotations and shapes for every axis, doesn't scale very well.

Improvements in -> 66df51e

alexcjohnson · 2018-07-26T19:44:45Z

src/plots/plots.js

    for(i = 0; i < fullData.length; i++) {
        trace = fullData[i];
+
+        // find array attributes in trace


🤔 isn't PlotSchema.findArrayAttributes(trace) self-documenting enough?

good call -> d0699c0

alexcjohnson · 2018-07-26T19:54:39Z

src/components/annotations/calc_autorange.js

+    var apos = ann['a' + letter];
+    var ref = ann[letter + 'ref'];
+    var aref = ann['a' + letter + 'ref'];
+    var shift = ann[letter + 'shift'];


if you did * (letter === 'x' ? 1 : -1) the switch below would collapse.

yes -> d0699c0

alexcjohnson · 2018-07-26T20:07:16Z

src/plots/cartesian/axes.js

@@ -50,6 +50,7 @@ axes.getFromTrace = axisIds.getFromTrace;
 var autorange = require('./autorange');
 axes.expand = autorange.expand;


🔪 in d0699c0

alexcjohnson · 2018-07-26T20:20:53Z

src/plots/cartesian/constraints.js

@@ -143,15 +144,17 @@ exports.enforce = function enforceAxisConstraints(gd) {
                        var newVal;
                        var k;

-                        for(k = 0; k < ax._min.length; k++) {
-                            newVal = ax._min[k].val - getPad(ax._min[k]) / m;
+                        var minArray = concatExtremes(gd, ax, 'min');


Hmm, so looks like we can call concatExtremes twice with the same results? Could we cache? Like, clear it maybe during supplyDefaults, then in concatExtremes first look for the cached value before calculating it and setting the cache?

Some improvements in 66df51e. concatExtremes is still called twice on graphs with constrained axes, so would do better if we want.

alexcjohnson · 2018-07-26T20:30:20Z

test/jasmine/tests/bar_test.js

+        Plotly.plot(gd, [
+            {type: 'bar', x: [1, 2, 3], y: [1, 2, 1]},
+            {type: 'bar', x: [1, 2, 3], y: [-1, -2, -1]}
+        ])


Is there a test with stacked bars, where the extremes of one trace depend on the traces before it (so may need to be recalculated if an earlier trace changes visibility)?

Is there a test with stacked bars,

Thanks for writing this down. Visibility restyles are broken on stacked bars, but for a different reason: Bar.setPositions mutates calcdata[i][j].b which is first set in Bar.calc 🙃

where the extremes of one trace depend on the traces before it

Yep, that's exactly why commit 4c250c3 is very important, 'plot' edit types now call setPositions for this reason.

fixed and 🔒 in f1cda60

alexcjohnson · 2018-07-26T20:48:46Z

src/plots/cartesian/autorange.js

+            if(Registry.traceIs(trace, 'cartesian') || Registry.traceIs(trace, 'gl2d')) {
+                var axId = ax._id;
+                if(extremes[axId]) {
+                    out = out.concat(extremes[axId][ext]);


In principle we can collapse out more than just concat - new items can be mooted or can moot existing items based on the three criteria used in expand/findExtremes

I suppose it's worth testing with some real examples though - doAutoRange scales as O(min.length * max.length) which says it's a good idea to minimize the length of the min and max arrays, but excluding items from the min and max arrays scales as something like O(finalArray.length * uncompressedTotalLength) which is also sort of quadratic... not exactly a clear win either way.

Actually there’s a probably much more common case where I’m pretty sure it’s a big win to collapse more here: lots of traces each with only one or two min/max entries. Like 100 line or bar traces, or 100 marker traces with constant marker size. Collapsing here takes ~100 * 2 comparisons, then doAutorange only has one calculation to do, whereas simple concat here leaves 100 * 100 calculations for doAutorange

alexcjohnson · 2018-07-26T20:55:38Z

one polar mock is currently failing, but I think it is more correct now.

I think I agree - the new one (radial autorange still happens even though angular data is invalid) matches how we handle cartesian with invalid data on the opposite axis, right?

... to keep track of traces, annotations and shapes plotted on axis ax. Adapt concatExtremes accordingly!

- Bar.setPositions mutate 'b' in bar trace calcdate, we need to reinit 'b' during setPositions so that we can skip calc on visible edits

etpinard · 2018-07-30T18:12:08Z

I'm starting to think we should abandon this PR and #2837 😢

Consider

Plotly.newPlot(gd, [{
  visible: false,
  y: [1, 2, 1]
}])

Plotly.restyle(gd, 'visible', true)

on this branch, this snippet shows a blank graph. That's because data arrays aren't coerced for visible:false traces and _module.calc is skipped for visible!==true traces.

So, one way to possibly save this PR is to make _module.calc run on 'legendonly' traces (note that data arrays are coerced for 'legendonly' traces) and make visible restyle calls on visible:false traces go through the 'calc' edit pipeline. Performance improvements would still be detected on legend toggling (where we toggle between visible 'legendonly' and true) - which is really the main purpose of this PR.

- we would need to coerce their xref and yref even when `visible:false`. Not worth the effort at the moment. This reverts commit 9a78013.

... to handle `visible:false` annotations and shapes that do not go through calcAutorange at the moment.

... by factoring out logic from findExtremes

etpinard · 2018-07-31T19:38:03Z

Abandoned. Recycled commits in Abandoned. See #2860

etpinard added 19 commits July 25, 2018 18:34

add axis.range under visible edits test

67db333

add findExtremes

3e817f3

- a Axes.expend clone that does not append things to ax._min/ax._max, but instead returns two arrays, a min array and max array of potential data extremes

add _extremes to all traces in calc

1cd8482

- to be filled with the results of findExtremes

adapt getAutoRange and doAutoRange to trace _extremes

e9d3ca0

- pass gd, to look for _extremes in gd._fullData and eventually in layout container to can expand the axis ranges

generalize concatExtremes for polar, splom and layout components

2966cf3

- N.B. sploms are linked to multiple axes per traces, so we can't rely on single 'x' and 'y' in _extremes -> use ax._id instead!

replace Axes.expand -> findExtremes in traces/

a9cf2e1

replace Axex.expand -> findExtremes in annotations and shapes

13cb0f0

replace Axes.expand -> findExtremes for ErrorBars

d24eaef

- here we append the min/max arrays of the corresponding trace

🔪 ax._min / ax._max logic for rangeslider

d9bb617

- questionable commit, especially the part in autorange.js, but this doesn't make any test fail ?!?

adapt enforceConstraints to new per trace/item _extremes

9b02ac4

adapt polar to new per trace/item _extremes

d407c90

- N.B. polar is the only subplot apart from cartesian that used Axes.expand and friends.

🔪 obsolete comments about ax._min / ax._max

9ab0c2f

adapt gl2d to findExtremes

6e866c2

🔪 Axes.expand

c45427b

adapt test for Axex.expand -> findExtremes change

e77cacc

adapt test to new getAutoRange API

ba3c903

sub fail -> failTest

424f4a6

setPositions even when recalc===false

4c250c3

udpdate jsdoc for Axes.getAutoRange

5becba0

etpinard added type: maintenance labels Jul 26, 2018

etpinard added this to the v1.40.0 milestone Jul 26, 2018

etpinard commented Jul 26, 2018

View reviewed changes

alexcjohnson reviewed Jul 26, 2018

View reviewed changes

etpinard added 4 commits July 27, 2018 15:00

use ax.(_traceIndices, annIndices, shapeIndices)

66df51e

... to keep track of traces, annotations and shapes plotted on axis ax. Adapt concatExtremes accordingly!

fixups (from AJ's review)

d0699c0

add bar stack visible -> autorange test & move 'b' init to setPositions

f1cda60

- Bar.setPositions mutate 'b' in bar trace calcdate, we need to reinit 'b' during setPositions so that we can skip calc on visible edits

make annotations & shapes 'visible' -> 'plot' edit type

9a78013

etpinard added 3 commits July 30, 2018 14:13

Revert "make annotations & shapes 'visible' -> 'plot' edit type"

5cfee13

- we would need to coerce their xref and yref even when `visible:false`. Not worth the effort at the moment. This reverts commit 9a78013.

add fallback for _extremes during concat

62f1549

... to handle `visible:false` annotations and shapes that do not go through calcAutorange at the moment.

collapse trace extremes before getAutorange

aac11a2

... by factoring out logic from findExtremes

etpinard mentioned this pull request Jul 31, 2018

Recycled commits from abandoned fast trace toggle PRs #2860

Merged

etpinard closed this Jul 31, 2018

etpinard deleted the per-trace-ax-extremes branch July 31, 2018 19:38

etpinard mentioned this pull request Jan 21, 2019

Bar & ErrorBar autorange fixes #3452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per-trace axis extremes #2849

Per-trace axis extremes #2849

etpinard commented Jul 26, 2018 •

edited

Loading

etpinard Jul 26, 2018

etpinard Jul 27, 2018

alexcjohnson Jul 26, 2018

etpinard Jul 27, 2018

alexcjohnson Jul 26, 2018

etpinard Jul 27, 2018

alexcjohnson Jul 26, 2018

etpinard Jul 27, 2018

alexcjohnson Jul 26, 2018

etpinard Jul 27, 2018 •

edited

Loading

alexcjohnson Jul 26, 2018

etpinard Jul 27, 2018 •

edited

Loading

etpinard Jul 27, 2018 •

edited

Loading

alexcjohnson Jul 26, 2018

alexcjohnson Jul 26, 2018

alexcjohnson commented Jul 26, 2018

etpinard commented Jul 30, 2018

etpinard commented Jul 31, 2018

		@@ -50,6 +50,7 @@ axes.getFromTrace = axisIds.getFromTrace;
		var autorange = require('./autorange');
		axes.expand = autorange.expand;

Per-trace axis extremes #2849

Per-trace axis extremes #2849

Conversation

etpinard commented Jul 26, 2018 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

etpinard Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Jul 26, 2018

etpinard commented Jul 30, 2018

etpinard commented Jul 31, 2018

etpinard commented Jul 26, 2018 •

edited

Loading

etpinard Jul 27, 2018 •

edited

Loading

etpinard Jul 27, 2018 •

edited

Loading

etpinard Jul 27, 2018 •

edited

Loading