Recycled commits from abandoned fast trace toggle PRs #2860

etpinard · 2018-07-31T19:37:14Z

based of #2823, supersedes #2837 and #2849.

I decided to abandoned the fast trace toggle PRs after #2849 (comment) and realizing that even a special 'legendonly' pathway wouldn't be enough (it would break categorical axes).

So, this PR attempts to ♻️ some of the work done in the past week, some of which have small but noticeable performance benefits.

Namely,

fullLayout._modules now includes trace modules of visible!==true traces. In particular, this leads to fullLayout._has('gl') returning true and some performance benefits for scattergl traces. For example,

Plotly.restyle(gd, 'visible', false)
setTimeout(() => {
  console.time('1')
  Plotly.restyle(gd, 'visible', true)
  console.timeEnd('1')
}, 500)

is 100ms faster now for scattergl graphs (no matter the number of points) as we no longer clear and reinit a WebGL context.

Axes.expand and ax._min/ax._max are replaced by findExtremes and _extremes containers in full trace and annotations/shapes items. Arguably, this improves maintenance as findExtremes does not mutate the full axis objects; it should make it easier to reuse cartesian/autorange.js in other subplot types. Moreover, this appears to have some performance benefits, perhaps as the pseudo O(n^2) expand/findExtremes algo is now done over a per-trace-not-axis-wide sample. For example, with

// this is the largest N before we use the average marker.size
// one Axes.expand / findExtremes call here clocks in at 20-50ms on average
var N = 1e5-1
var N2 = 1e5-1

var x = []
var y = []
var ms = []
var x2 = []
var y2 = []
var ms2 = []
var i

for(i = 0; i < N; i++) {
  x.push(Math.random())
  y.push(Math.random())
  ms.push(i % 2 ? 20 : 10)
}
for(i = 0; i < N2; i++) {  
  x2.push(Math.random() + 2)
  y2.push(Math.random())
  ms2.push(i % 2 ? 10 : 20)
}

Plotly.newPlot('graph', [{
  type: 'scattergl',
  mode: 'markers',
  x: x, y: y,
  marker: {size: ms}
}, {
  type: 'scattergl',
  mode: 'markers',
  x: x2, y: y2,
  marker: {size: ms2}
}])

The sum of the findExtremes computation times (2 per x/y axes) appear 10ms faster than the corresponding Axes.expand calls on master. Note that Axes.getAutorange is 10x slower on this branch (most likely from concatenating trace _extremes), but still clocks in well below the 1ms mark.

finally, this PR adds several tests the assert axis range after visible restyles.

But, please note this PR does not attempt to make trace and annotations/shapes visible a 'plot' edit type. They will remain a 🐢 'calc' edit type for the next few months 😞

cc @alexcjohnson

- DRY using .plot & .cleear with getViewport helper fn - Dry .plot and .update with repeat helper fn - compute viewport only once per subplot, as opposed to once per trace.

- so that gl-based trace can call their plot methods w/ an empty array of traces and just work. - update and improve scatterlg visibility tests to reflect that `restyle(gd,visible,false)` no longer clear the context

- to not have to guard against visible!==true traces in _module.style - to shortcut full list of modules in other places downstream

- Bar.setPositions mutates 'b' in bar trace calcdata, so to reinit 'b' during setPositions so that it doesn't conflit with Bar.calc

- 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

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

- replacing Axe.expand - for ErrorBars, 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.

- by using ax.(_traceIndices, annIndices, shapeIndices), to keep track of traces, annotations and shapes plotted on axis ax. Adapt concatExtremes accordingly!

... by factoring out logic from findExtremes

etpinard · 2018-07-31T19:43:42Z

Oh and I kept the work of f1cda60 squashed in dfada6a that moved the bar calc 'bar' mesures (i.e. calcdata[i][j].b) from Bar.calc to Bar.setPositions so that Bar.setPositions doesn't mutate what Bar.calc computed.

More info off #2849 (comment)

alexcjohnson · 2018-07-31T20:35:22Z

src/traces/scattergl/index.js

+        opts[i] = opt;
+    }
+    return opts;
+}


non blocking, would probably make a good Lib function. Pretty sure we use this in other places as well, though I'm not quite sure how to search for it...

done in 33b4085

I didn't find any other usage of a repeat helper in src/, but making a lib function now should help us remember that this helper exists.

alexcjohnson · 2018-07-31T20:37:09Z

src/plots/get_data.js

@@ -69,6 +69,7 @@ exports.getModuleCalcData = function(calcdata, arg1) {
    for(var i = 0; i < calcdata.length; i++) {
        var cd = calcdata[i];
        var trace = cd[0].trace;
+        // N.B. 'legendonly' traces do not make it pass here


~~pass~~ past

fixed in 89aebd1

alexcjohnson · 2018-07-31T21:56:18Z

src/traces/bar/set_positions.js

+    for(var i = 0; i < cd.length; i++) {
+        cd[i][0].trace._extremes[ax._id] = extremes;
+    }
+}


Now that we're not trying to make _extremes survive visibility changes, does it actually help to have the same thing repeated in all traces here? Seems like that will just waste time later on. Can we just put it in the first one and clear all the others?

Thanks for pointing this out, but turns out we still need to attach _extremes to all bar traces at the moment for error bars. Error bars concat their min/max extremes into the trace._extremes in which they are linked and assume that trace._extremes[ax._id] exists.

We could alternatively linked error bar _extremes in their trace.error_(x|y) containers and make getAutorange look for error_(x|y) in each trace. Or perhaps, making error bar create trace._extremes[ax._id] containers when needed would suffice.

But, I wouldn't worry too much about performance. I can't imagine getAutorange taking more than 1ms even in the worse of scenario.

Ah OK. I suppose the first one could have the actual values, then all the others could just get [] to give something for errorbars to append to... but yeah, no big deal, no block. These extras will get pruned quickly during your improved _concat that I hadn't noticed when I made that comment.

alexcjohnson · 2018-07-31T21:58:06Z

src/traces/box/set_positions.js

+        // set the width of all boxes
+        calcTrace[0].t.dPos = dPos;
+        // link extremes to all boxes
+        calcTrace[0].trace._extremes[posAxis._id] = extremes;


same question as for bars... can we just put these extremes in the first one?

... for consistency I'm leaning on voting for making all traces have an _extremes container. Like in my last comment, I can't imagine getAutorange ever being a performance drag.

alexcjohnson · 2018-07-31T22:05:26Z

src/plots/cartesian/autorange.js

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


I think my #2849 (comment) and #2849 (comment) still hold, and we should prune these the same way as in findExtremes to minimize O(min.length * max.length) in doAutoRange.

oh sorry, you did that in a0bfaf3 🎉 and also 🌴 🏆 💯

Yeah I tried to squash a0bfaf3 and 72f06a6 into ad1ac1f, but that was too much of a 🤕

alexcjohnson · 2018-07-31T22:25:30Z

src/plots/polar/layout_defaults.js

@@ -55,6 +55,7 @@ function handleDefaults(contIn, contOut, coerce, opts) {
        // propagate the template.
        var axOut = contOut[axName] = {};
        axOut._id = axOut._name = axName;
+        axOut._traceIndices = subplotData.map(function(t) { return t.index; });


In cartesian you use t._expandedIndex - should it be the same here?

Good eye. Thanks!

fixed and 🔒 in d233f3b

alexcjohnson · 2018-07-31T22:41:42Z

Great job plucking out the good bits, this still ended up as a pretty big PR, that cleans up a lot of things! Should we give it a type: bug as well? It still includes the splom fix #2837 (comment)

etpinard · 2018-08-01T14:03:18Z

It still includes the splom fix #2837 (comment)

Yes, it does. Thanks!

alexcjohnson · 2018-08-01T16:16:56Z

Beautiful. 🍋 -> 🍹
💃

Merging #2860 to master

etpinard added 18 commits July 31, 2018 14:21

DRY and small pref boost for scattergl

69dc0c4

- DRY using .plot & .cleear with getViewport helper fn - Dry .plot and .update with repeat helper fn - compute viewport only once per subplot, as opposed to once per trace.

push trace module into fullLayout._modules even if visible:false

41ad08a

- so that gl-based trace can call their plot methods w/ an empty array of traces and just work. - update and improve scatterlg visibility tests to reflect that `restyle(gd,visible,false)` no longer clear the context

fill in list of visible:true module in fullLayout._visibleModules

9c8ba02

- to not have to guard against visible!==true traces in _module.style - to shortcut full list of modules in other places downstream

fix and 🔒 splom trace visible toggling

8ef5cb3

sub fail -> failTest

cf0b19d

add scatter visibility restyles tests

9cc5fbe

add bar autorange tests & move 'b' init to setPositions

dfada6a

- Bar.setPositions mutates 'b' in bar trace calcdata, so to reinit 'b' during setPositions so that it doesn't conflit with Bar.calc

add findExtremes

be44366

- 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

adapt getAutoRange and doAutoRange to trace _extremes

ad1ac1f

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

fill trace._extremes with findExtremes in calc

769c160

- replacing Axe.expand - for ErrorBars, we append the min/max arrays of the corresponding trace

replace Axex.expand -> findExtremes in annotations and shapes

736ab69

🔪 ax._min / ax._max logic for rangeslider

6194457

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

adapt enforceConstraints to new per trace/item _extremes

8cd06ae

adapt polar to new per trace/item _extremes

2a745de

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

adapt gl2d to findExtremes

82d4bcc

🔪 Axes.expand & adapt test for findExtremes

29db388

improve concatExtremes perf

72f06a6

- by using ax.(_traceIndices, annIndices, shapeIndices), to keep track of traces, annotations and shapes plotted on axis ax. Adapt concatExtremes accordingly!

collapse trace extremes before getAutorange

a0bfaf3

... by factoring out logic from findExtremes

etpinard added type: maintenance labels Jul 31, 2018

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

This was referenced Jul 31, 2018

Faster trace visibility toggling #2837

Closed

Per-trace axis extremes #2849

Closed

alexcjohnson reviewed Jul 31, 2018

View reviewed changes

etpinard added the bug something broken label Aug 1, 2018

etpinard added 3 commits August 1, 2018 11:06

mv repeat -> Lib.repeat

33b4085

fix and 🔒 _extremes in polar tranformed traces

d233f3b

fix typos in 📚

89aebd1

etpinard merged commit 20db59a into faster-axis-autorange Aug 2, 2018

etpinard deleted the per-trace-ax-extremes-maintenance branch August 2, 2018 15:59

etpinard mentioned this pull request Aug 2, 2018

Merging #2860 to master #2865

Merged

etpinard added a commit that referenced this pull request Aug 2, 2018

Merge pull request #2865 from plotly/faster-axis-autorange

6a6d1dc

Merging #2860 to master

etpinard mentioned this pull request Sep 4, 2018

Stacked Area Charts #2960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recycled commits from abandoned fast trace toggle PRs #2860

Recycled commits from abandoned fast trace toggle PRs #2860

etpinard commented Jul 31, 2018

etpinard commented Jul 31, 2018 •

edited

Loading

alexcjohnson Jul 31, 2018

etpinard Aug 1, 2018

etpinard Aug 1, 2018

alexcjohnson Jul 31, 2018

etpinard Aug 1, 2018

alexcjohnson Jul 31, 2018

etpinard Aug 1, 2018

alexcjohnson Aug 1, 2018

alexcjohnson Jul 31, 2018

etpinard Aug 1, 2018

alexcjohnson Jul 31, 2018

alexcjohnson Jul 31, 2018

etpinard Aug 1, 2018 •

edited

Loading

alexcjohnson Jul 31, 2018

etpinard Aug 1, 2018

etpinard Aug 1, 2018

alexcjohnson commented Jul 31, 2018

etpinard commented Aug 1, 2018

alexcjohnson commented Aug 1, 2018

Recycled commits from abandoned fast trace toggle PRs #2860

Recycled commits from abandoned fast trace toggle PRs #2860

Conversation

etpinard commented Jul 31, 2018

etpinard commented Jul 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Aug 1, 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

alexcjohnson commented Jul 31, 2018

etpinard commented Aug 1, 2018

alexcjohnson commented Aug 1, 2018

etpinard commented Jul 31, 2018 •

edited

Loading

etpinard Aug 1, 2018 •

edited

Loading