Even better cartesian trace updates and removal #2579

etpinard · 2018-04-24T22:06:28Z

to be merged in #2574.

This PR implements @alexcjohnson 's idea (:pencil2: down in #2563 (comment)) of making cartesian trace module layers (e.g. <g.scatterlayer>, <g.barlayer>, etc) data driven. Getting this to work was easy. Getting this to work well -- that is making sure layers don't get removed/added when they don't need to -- was a little trickier. Why? In brief d3's select() can mutate node data in some scenario (see example). This is not the first time we noticed this behavior. Ricky once wrote:

plotly.js/src/traces/scattercarpet/plot.js

Lines 33 to 34 in 4ed586a

    
           // Note: .select is adequate but seems to mutate the node data, 
        
           // which is at least a bit suprising and causes problems elsewhere

Oh well, see 284c206 for more details.

This PR also makes heatmap/contour/carpet plot methods know how to remove old traces in da63ed4. These modules don't use selectAll().data() to build multiple traces. They instead use trace.uid to build and keep track of nodes. Previously, these old nodes were removed by looping over all traces and using some pretty nasty selectAll(). This significantly improves redraw perf on large (multi-subplot, multi-trace) graphs. For example, using this script with 30x30 subplots/traces and calling relayout(gd, 'xaxis.range', [/*any*/]) used to take ~8000ms, and now is under ~3000ms (which is still slow, but hey it's a start 😜 ).

- should fix some intermittent hangups when running the image tests locally

- make trace module layers 'data-driven', one layer per unique plot module. Use _module.layerName to denote reused plot methods. - pass layer to trace _module.plot (as 3rd arg) to not mutate node data and allow a given _module.plot method to trace in multiple layers - no need for previous `_plotMethods` stash attempt - rename 'imagelayer' -> 'heatmaplayer', 'maplayer' -> 'contourlayer' for consistency for other trace modules

... instead of relying on slow selectAll() during subroutines.drawData and Plots.cleanPlot. This can be a major perf improvement on graph with many DOM nodes (e.g. from many traces and/or many subplots)

... to take into account take now not all trace module layers are added to each subplot.

- previously, Contour.plot called Heatmap.call for traces with 'contour.coloring': 'heatmap' which plotted a heatmap in <g.imagelayer> always below the contours. Now as <g.heatmaplayer> does not always exist, we plot the heatmap fill inside <g.contourlayer> in a ensureSingle layer.

etpinard · 2018-04-24T22:10:14Z

src/traces/histogram2d/index.js

@@ -15,6 +15,7 @@ Histogram2D.attributes = require('./attributes');
 Histogram2D.supplyDefaults = require('./defaults');
 Histogram2D.calc = require('../heatmap/calc');
 Histogram2D.plot = require('../heatmap/plot');
+Histogram2D.layerName = 'heatmaplayer';


N.B. Trace modules that reuse a plot method verbatim from another module need to declare layerName so that their traces are combined into the same trace module layer.

.... there's probably a way to 🔪 layerName entirely by building some trace-to-layerName map object within the Registry, but oh well I didn't think adding a few of ⤴️ would be too bad. Thoughts?

I like it this way - explicit and clear.

etpinard · 2018-04-24T22:10:43Z

src/plots/cartesian/constants.js

        'barlayer',
        'carpetlayer',
        'violinlayer',
        'boxlayer',
        'ohlclayer',
-        'scatterlayer'
+        'scattercarpetlayer', 'scatterlayer'


N.B. On the other hand, trace modules (e.g. scattercarpet) that only wrap another plot method need a separate layer.

is there a reason some of the new ones share lines?

I put modules that almost have the same plot method on the same line. I thought that would make it easier to track. I can change that of course.

etpinard · 2018-04-24T22:13:51Z

src/traces/contour/plot.js


    // use a heatmap to fill - draw it behind the lines
+    var heatmapColoringLayer = Lib.ensureSingle(contourLayer, 'g', 'heatmapcoloring');


See 48ee0d8 for explanation.

alexcjohnson · 2018-04-25T16:18:32Z

tasks/util/container_commands.js

+        cmd = getRunCI(_commands);
+    } else {
+        _commands = [containerCommands.setup].concat(_commands);
+        cmd = getRunLocal(_commands);


so if I'm reading this right, locally we'll be restarting, pinging, and waiting 5 seconds at the beginning of every command? Seems a little heavy, but if it avoids random hangs (which waste a lot more than 5 secs when they happen) then it's OK.

Yeah it's a little heavy, but at least now npm run test-image always works^TM.

I should probably give #1972 another go at some point in the next few weeks. We really don't need to be running pixel comparison tests with a server, a CLI runner should do just fine.

alexcjohnson · 2018-04-25T16:32:24Z

src/plots/cartesian/index.js

+                        cdModule: cdModule
+                    });
+                }
+                break;


Does this need to be a 2D loop? Could we just loop over modules, include the layer index in layerData, and sort by that at the end? I guess we'd also need to track which layers we've already included, to ensure uniqueness. But still seems faster than looping over all combinations of className and _module.

I wanted to avoid sorting for 🐎 but you're probably right.

done in d47276f

alexcjohnson · 2018-04-25T16:44:24Z

re: tricky .select() behavior - see d3/d3#1443 (comment)

I've added an extension to d3-selection, selection.selectWithoutDataPropagation which does d3.select(this.node().querySelector(name))

In case using selectAll in these instances becomes a 🐎 issue or something...

alexcjohnson · 2018-04-25T16:50:12Z

src/traces/heatmap/plot.js

-    for(var i = 0; i < cdheatmaps.length; i++) {
-        plotOne(gd, plotinfo, cdheatmaps[i]);
+    heatmapLayer.selectAll('.hm > image').each(function(d) {
+        var oldTrace = d.trace || {};


when is there no d.trace?

when a heatmap trace is completely off screen:

plotly.js/src/traces/heatmap/plot.js

Lines 138 to 142 in bab2a5b

var isOffScreen = (imageWidth <= 0 || imageHeight <= 0);

var plotgroup = plotinfo.plot.select('.imagelayer')

.selectAll('g.hm.' + id)

.data(isOffScreen ? [] : [0]);

This is not a big deal, but wouldn't an off-screen heatmap just not contribute to the '.hm > image' selection at all?

removing that || {} makes

plotly.js/test/jasmine/tests/heatmap_test.js

Lines 482 to 506 in c0b38da

it('should not draw traces that are off-screen', function(done) {

var mock = require('@mocks/heatmap_multi-trace.json'),

mockCopy = Lib.extendDeep({}, mock),

gd = createGraphDiv();

function assertImageCnt(cnt) {

var images = d3.selectAll('.hm').select('image');

expect(images.size()).toEqual(cnt);

}

Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {

assertImageCnt(5);

return Plotly.relayout(gd, 'xaxis.range', [2, 3]);

}).then(function() {

assertImageCnt(2);

return Plotly.relayout(gd, 'xaxis.autorange', true);

}).then(function() {

assertImageCnt(5);

done();

});

});

fail with

... I didn't investigate further.

Alright, not what my quick glance at the code suggested but you're right, no need to dig in any further.

alexcjohnson · 2018-04-25T16:55:30Z

src/traces/carpet/plot.js

+        var oldUid = classString.split('carpet')[1].split(/\s/)[0];
+        for(i = 0; i < cdcarpet.length; i++) {
+            if(oldUid === cdcarpet[i][0].trace.uid) return;
+        }


🌴 🐎 perhaps make a common helper getUidsFromCalcdata or something, that can be called once up front and returns a hash {uid0: 1, uid1: 1, ...}, then if(!uidHash[oldUid]) d3.select(this).remove();?
Looks like the same helper would work for all modules in this commit.

done in d47276f

alexcjohnson · 2018-04-25T17:09:51Z

@etpinard this is amazing - you make it look so easy, I thought it would be way more involved 😅
Just a few 🐎 suggestions/questions, then this'll be ready to go!

- I don't see the need to optimize here using e.g a lookup object. traceLayerClasses.length ~ 10, and fullLayout._modules.length < 20, most often 1 or 2.

alexcjohnson · 2018-04-30T15:23:31Z

.circleci/test.sh

@@ -34,7 +34,7 @@ case $1 in

    jasmine2)
        retry npm run test-jasmine -- --tags=gl --skip-tags=noCI,flaky
-        retry npm run test-jasmine -- --tags=flaky --skip-tags=noCI
+        retry npm run test-jasmine -- --tags=flaky --skip-tags=noCI,gl


wait, then will tests marked @flaky AND @gl run at all on CI?

reverted in 8c9a7bf

etpinard · 2018-04-30T15:23:43Z

@alexcjohnson thanks for digging up d3/d3#1443 (comment)! I guess that's something we'll just have to in mind going forward as we make our update patterns more d3-esque.

@gl

- so that we cover tests that are tagged @gl and @flaky This reverts commit 83422f7.

alexcjohnson · 2018-04-30T15:46:17Z

Beautiful - thanks for those couple of changes. This is a huge improvement not just in 🐎 but in structure and maintainabilty! 💃

etpinard added 7 commits April 24, 2018 17:17

ping imagetest after having restarting nw.js

400bfe6

- should fix some intermittent hangups when running the image tests locally

make heatmap/contour(carpet)/carpet plot method remove its own trace

da63ed4

... instead of relying on slow selectAll() during subroutines.drawData and Plots.cleanPlot. This can be a major perf improvement on graph with many DOM nodes (e.g. from many traces and/or many subplots)

sub imagelayer->heatmaplayer, maplayer->contourlayer in tests

a1c76bc

update cliponaxis: false

fa38ca5

... to take into account take now not all trace module layers are added to each subplot.

🔒 scattercarpet + scatter coexistence

362da1f

etpinard added type: maintenance labels Apr 24, 2018

etpinard commented Apr 24, 2018

View reviewed changes

etpinard added this to the 1.37.0 milestone Apr 24, 2018

alexcjohnson reviewed Apr 25, 2018

View reviewed changes

etpinard added 3 commits April 30, 2018 11:03

add getUidsFromCalcData helper

3bef9a9

make visible trace module -> layer data a 1D loop w/ + indexOf + sort

d47276f

- I don't see the need to optimize here using e.g a lookup object. traceLayerClasses.length ~ 10, and fullLayout._modules.length < 20, most often 1 or 2.

skip gl tags in flaky-tagged jasmine test cmd

83422f7

alexcjohnson reviewed Apr 30, 2018

View reviewed changes

Revert "skip gl tags in flaky-tagged jasmine test cmd"

8c9a7bf

- so that we cover tests that are tagged @gl and @flaky This reverts commit 83422f7.

etpinard merged commit f771310 into cartesian-trace-deletion Apr 30, 2018

etpinard deleted the uid-trace-removal branch April 30, 2018 16:02

etpinard mentioned this pull request Jul 5, 2018

Fix box & violin inner parts removal #2785

Merged

etpinard mentioned this pull request Jul 6, 2018

Better axis range animations #2788

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Even better cartesian trace updates and removal #2579

Even better cartesian trace updates and removal #2579

etpinard commented Apr 24, 2018

etpinard Apr 24, 2018

etpinard Apr 24, 2018 •

edited

Loading

alexcjohnson Apr 25, 2018

etpinard Apr 24, 2018

alexcjohnson Apr 25, 2018

etpinard Apr 30, 2018 •

edited

Loading

etpinard Apr 24, 2018

alexcjohnson Apr 25, 2018

etpinard Apr 30, 2018

alexcjohnson Apr 25, 2018

etpinard Apr 30, 2018

etpinard Apr 30, 2018

alexcjohnson commented Apr 25, 2018

alexcjohnson Apr 25, 2018

etpinard Apr 30, 2018

alexcjohnson Apr 30, 2018

etpinard Apr 30, 2018

etpinard Apr 30, 2018

alexcjohnson Apr 30, 2018

alexcjohnson Apr 25, 2018

etpinard Apr 30, 2018

alexcjohnson commented Apr 25, 2018

alexcjohnson Apr 30, 2018

etpinard Apr 30, 2018

etpinard commented Apr 30, 2018

alexcjohnson commented Apr 30, 2018

	// Note: .select is adequate but seems to mutate the node data,
	// which is at least a bit suprising and causes problems elsewhere


		// use a heatmap to fill - draw it behind the lines
		var heatmapColoringLayer = Lib.ensureSingle(contourLayer, 'g', 'heatmapcoloring');

	var isOffScreen = (imageWidth <= 0 \|\| imageHeight <= 0);

	var plotgroup = plotinfo.plot.select('.imagelayer')
	.selectAll('g.hm.' + id)
	.data(isOffScreen ? [] : [0]);

	it('should not draw traces that are off-screen', function(done) {
	var mock = require('@mocks/heatmap_multi-trace.json'),
	mockCopy = Lib.extendDeep({}, mock),
	gd = createGraphDiv();

	function assertImageCnt(cnt) {
	var images = d3.selectAll('.hm').select('image');

	expect(images.size()).toEqual(cnt);
	}

	Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
	assertImageCnt(5);

	return Plotly.relayout(gd, 'xaxis.range', [2, 3]);
	}).then(function() {
	assertImageCnt(2);

	return Plotly.relayout(gd, 'xaxis.autorange', true);
	}).then(function() {
	assertImageCnt(5);

	done();
	});
	});

Even better cartesian trace updates and removal #2579

Even better cartesian trace updates and removal #2579

Conversation

etpinard commented Apr 24, 2018

Choose a reason for hiding this comment

etpinard Apr 24, 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

etpinard Apr 30, 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

alexcjohnson commented Apr 25, 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

alexcjohnson commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Apr 30, 2018

alexcjohnson commented Apr 30, 2018

etpinard Apr 24, 2018 •

edited

Loading

etpinard Apr 30, 2018 •

edited

Loading