Splom zoom perf #2527

etpinard · 2018-04-05T20:25:14Z

etpinard · 2018-04-05T20:32:21Z

src/plots/plots.js

-            return true;
-        }
+        var name = modules[i].name;
+        if(name === category) return true;


so calls like fullLayout._has('scattergl') just works w/o having to set a scattergl category in the trace module.

alexcjohnson · 2018-04-05T20:43:24Z

src/plots/cartesian/graph_interact.js

+// should trigger a full initInteractions.
+exports.updateFx = function(fullLayout) {
+    var cursor = fullLayout.dragmode === 'pan' ? 'move' : 'crosshair';
+    setCursor(fullLayout._draggers, cursor);


Ah, great solution - much simpler than what I was thinking, and at least as fast 🐎

etpinard · 2018-04-05T20:44:20Z

src/traces/scatter/index.js

@@ -34,7 +34,7 @@ Scatter.animatable = true;
 Scatter.moduleType = 'trace';
 Scatter.name = 'scatter';
 Scatter.basePlotModule = require('../../plots/cartesian');
-Scatter.categories = ['cartesian', 'symbols', 'markerColorscale', 'errorBarsOK', 'showLegend', 'scatter-like'];
+Scatter.categories = ['cartesian', 'svg', 'symbols', 'markerColorscale', 'errorBarsOK', 'showLegend', 'scatter-like', 'draggedPts'];


IMPORTANT (and possibly worth refactoring)

scattergl and splom traces are currently declared with a 'cartesian' category so that cartesian axis logic that they share just works. But this means that not all 'cartesian' traces are svg or more precisely not all 'cartesian' traces are plotted in <g .cartersianlayer>. So to avoid looping over all subplots on drag to shift plotinfo.plot and its clip <rect> for scattergl and splom traces, I made an 'svg' category. This is a slight misnomer though, as heatmaps and histogram2d are here considered svg even though they are built using a <canvas>. Oh well. The 'svg' category mirrors the 'gl' and 'regl' category listed in scattergl and splom trace modules. Any objections?

Moreover, I added a 'draggedPts' category to avoid those costly selectAll('.point') on pan when we don't need them. For example, graphs with contour lines feel faster on pan as we no longer traverse the <g .cartesianlayer> looking for points to scaled.

I think this is fine - 'cartesian' means "data relates to cartesian axes" and 'svg' means "data is drawn into an SVG container" (heatmaps use a canvas to generate the image but then put it into an svg <image> element)

'draggedPts' is still potentially overkill, as the trace type doesn't necessarily tell you if the traces contain these elements - all of these trace types can be drawn with no points or text, only lines and fills...

etpinard · 2018-04-05T20:47:00Z

src/plots/cartesian/axes.js

@@ -2188,7 +2188,7 @@ axes.doTicks = function(gd, axid, skipTitle) {
            }
            drawTicks(mainPlotinfo[axLetter + 'axislayer'], tickpath);

-            tickSubplots = Object.keys(ax._linepositions);
+            tickSubplots = Object.keys(ax._linepositions || {});


Unfortunate, but important as relinkPrivateKeys does relink empty objects.

... does not relink

does not relink empty objects.

alexcjohnson · 2018-04-05T20:52:22Z

src/plots/plots.js

+        var name = modules[i].name;
+        if(name === category) return true;
+        // N.B. this is modules[i] along with 'categories' as a hash object
+        var _module = Registry.modules[name];


TBH I've forgotten why we wrapped modules like this in the registry... not a big deal, but would it be better to attach the hash directly to the module, or even make categories as a hash from the beginning? Is it ever important that it's a list?

Referencing #2174 where we like to make building custom trace modules more user friendly:

we should try to not mutate the input module object, so that means not replacing the categories list with a hash object nor augmenting the input module object with e.g. a categoryHash key.

listing categories as an array feels more natural than {cat1: 1, cat2: 1} hash object

So, I'm voting for the status quo.

alexcjohnson · 2018-04-05T21:36:45Z

src/plot_api/plot_api.js

+                // TODO
+                // no test fail when commenting out doAutoRangeAndConstraints,
+                // but I think we do need this (maybe just the enforce part?)
+                // Am I right?


Huh, I'd have thought so... perhaps worth spending a little time looking for something that does fail if this is removed, so it can be 🔒 down.

@alexcjohnson Tests are currently passing even when subroutines.doAutoRangeAndConstraints is commented out, because of this block:

plotly.js/src/plot_api/plot_api.js

Lines 2088 to 2107 in 2a25820

// figure out if we need to recalculate axis constraints

var constraints = fullLayout._axisConstraintGroups || [];

for(axId in rangesAltered) {

for(i = 0; i < constraints.length; i++) {

var group = constraints[i];

if(group[axId]) {

// Always recalc if we're changing constrained ranges.

// Otherwise it's possible to violate the constraints by

// specifying arbitrary ranges for all axes in the group.

// this way some ranges may expand beyond what's specified,

// as they do at first draw, to satisfy the constraints.

flags.calc = true;

for(var groupAxId in group) {

if(!rangesAltered[groupAxId]) {

Axes.getFromId(gd, groupAxId)._constraintShrinkable = true;

}

}

}

}

}

which makes axis range relayout calls altering constrained axes go though the calc: true edit pathway which (of course) calls doAutoRangeAndConstraints itself.

Interestingly, commenting out that block which sets flags.calc = true does not make any test fails when subroutines.doAutoRangeAndConstraints is called during axrange edits. Perhaps we don't need?

It doesn't look to me as though I wrote all that great test coverage for this section... the two key points of it are:

You can relayout a constrained axis, particularly to a smaller range, and have the other axes in the group update (expanding or shrinking) to continue to meet the constraints (note that this won't work with react, there you'd have to manually update all axes in the constraint group)

If you relayout (or react) two or more axes in a group in such a way as to explicitly violate the constraints, one or more of those axes will expand (never shrink) its range to fit the constraints.

So if you want to clean this up we should first add a few tests explicitly for ^^. I see some gui tests and one restyle test that uses constraints, but unfortunately no relayout or react tests of these edge cases.

Ok if I open up an issue about this and defer this to a later PR?

Ok if I open up an issue about this and defer this to a later PR?

Yep for sure - that was the "if you want to clean this up" clause

Moved to -> #2540

alexcjohnson · 2018-04-05T22:01:40Z

alexcjohnson · 2018-04-10T21:44:45Z

src/components/shapes/draw.js

+
+    for(var k in fullLayout._plots) {
+        var shapelayer = fullLayout._plots[k].shapelayer;
+        if(shapelayer) shapelayer.selectAll('path').remove();


Interesting - why is this better? Wasn't fullLayout._shapeSubplotLayers essentially just an array of all the shapelayer nodes? Or is the win actually below in subroutines.js that you don't have to do a selectAll to find these?

Or is the win actually below in subroutines.js that you don't have to do a selectAll to find these?

That. selectAll are bad.

alexcjohnson · 2018-04-10T21:46:05Z

src/plots/cartesian/dragbox.js

@@ -1105,7 +1105,7 @@ function calcLinks(gd, xaHash, yaHash) {
    var yaHashLinked = {};
    var yaxesLinked = [];
    for(yLinkID in yLinks) {
-        var ya = getFromId(gd, xLinkID);
+        var ya = getFromId(gd, yLinkID);


yikes, good catch!

etpinard · 2018-04-11T14:41:44Z

alexcjohnson · 2018-04-11T19:55:26Z

etpinard added 7 commits April 5, 2018 14:25

move doAutoRangeAndConstraints, drawData and finalDraw to subroutines

02ed2eb

... (i.e. out of plot_api.js), and move Plo 8000 ts.addLinks out of drawData into its own step in Plotly.plot.

add 'axrange' editType

3ad1eaa

- this represents the minimal sequence for '(x|y)axis.range' relayout calls which are pretty common (e.g. on zoom/pan mouseup). - by bypassing drawFramework, lsInner and initInteraction, this can save ~1000ms on 50x50 subplot grids.

improve fullLayout._has()

d07ae70

- allow fullLayout._has(/*trace type*/) to work - use registry category hash object (instead of categories.indexOf) to find categories in fullLayout._modules

clean splom drag logic

0979272

lint in plot_api

ebb35ce

etpinard added status: in progress labels Apr 5, 2018

etpinard commented Apr 5, 2018

View reviewed changes

alexcjohnson reviewed Apr 5, 2018

View reviewed changes

etpinard commented Apr 5, 2018

View reviewed changes

alexcjohnson reviewed Apr 5, 2018

View reviewed changes

etpinard added 4 commits April 6, 2018 11:13

(fixup) add draggedPts to scattercarpet categories

f9090b7

Merge branch 'splom-feature' into splom-zoom-perf

b2ee736

Merge branch 'splom-feature' into splom-zoom-perf

a0b2574

replace selectAll with for(k in _plots) loop

d3fe40d

etpinard added this to the v1.36.0 milestone Apr 10, 2018

fixup and 🔒 dragbox with linked y axes

cc1b3de

alexcjohnson reviewed Apr 10, 2018

View reviewed changes

etpinard added 2 commits April 11, 2018 10:42

Merge branch 'splom-feature' into splom-zoom-perf

980855c

(to get tests to pass)

📚 link TODOs to gh issues

f7d637d

etpinard added status: reviewable and removed status: in progress labels Apr 11, 2018

etpinard merged commit 978a70e into splom-feature Apr 11, 2018

etpinard deleted the splom-zoom-perf branch April 11, 2018 20:44

etpinard mentioned this pull request Apr 13, 2018

Splom open items #2555

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Splom zoom perf #2527

Splom zoom perf #2527

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Splom zoom perf #2527

Splom zoom perf #2527

Uh oh!

Conversation

Things improved so far

Things I'll like to improve in the coming days

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!