Autosize / autorange fixes #2437

alexcjohnson · 2018-03-05T08:31:12Z

Fixes #2428 - despite its recent posting, I think this is one of our oldest bugs. You see it in the way a lot of autoranged plots will change just a little bit if you double-click them to cause the autorange to rerun... including a whole lot of our test images. They don't do that anymore. This happened when the margins get pushed automatically (mostly by the legend) and the axes are padded, because Axes.expand (called during calc) depended on ax._length, which could change after calc due to margin pushers. As a nice side benefit, now that Axes.expand is independent of axis length all the layout attributes that affect axis lengths can have their editType downgraded from calc to plot, speeding up all these edits.

Fixes #2385 as well - Plotly.react now handles autosize and size changes correctly. This was mostly just a matter of adding the special autosize handling that we already had in relayout into react as well, but then when I dropped axis size changes to editType: 'plot' it also required some more careful handling of ax._min/_max.

cc @etpinard

alexcjohnson · 2018-03-05T08:40:46Z

src/plot_api/plot_api.js

+        // // To do so, we must manually set them back here using the _initialAutoSize cache.
+        // if(['width', 'height'].indexOf(ai) !== -1 && vi === null) {
+        //     fullLayout[ai] = gd._initialAutoSize[ai];
+        // }


On closer inspection I think I understand what this is about, but I don't think it's actually the correct behavior, for example if gd was resized external to plotly.js it wouldn't make sense to revert to when the plot was first drawn. I'm not quite sure how to handle this though.

😬 auto-sizing always has a way to make my brain hurt.

Referencing #635 which did a lot of damage the streambed 🤕

Oy, I think we've all fought with that one at various points... If you don't mind, I'm going to leave this as it is now - with the TODO here to remind us about it if a related bug pops up, but without trying to implement anything around it. We have some great tests in config_test that I extended to make sure that all of those test cases work the same in relayout and react, I'd like to just freeze it until we have more test cases to look at.

alexcjohnson · 2018-03-05T08:43:34Z

src/plots/cartesian/autorange.js

+    doAutoRange: doAutoRange,
+    needsAutorange: needsAutorange,
+    expand: expand
+};


I initially pulled this module out of axes.js to avoid circular references; then I ended up moving clearCalc into setConvert instead of here, which mooted the circular reference problem, but I still like having these separate from the behemoth axes.js.

alexcjohnson · 2018-03-05T08:50:00Z

src/plots/gl2d/scene2d.js

@@ -261,7 +260,7 @@ proto.updateSize = function(canvas) {
    }

    // make sure plots render right thing
-    if(this.redraw) this.redraw();
+    // if(this.redraw) this.redraw();


Oh I forgot to remove this line... but @dfcreative @etpinard I wanted to ask about it anyway: if I leave it in, then occasionally - most noticeably when I autorange an old gl2d plot (ie pointcloud) - I get some extra tick labels: the new tick values get placed on the old axis range (if they're visible) then the correct tick labels get drawn over them without removing the obsolete ones.

Anyway I couldn't see any ill effects from removing it. OK?

Yeah, looks like in scene2d.plot this

plotly.js/src/plots/gl2d/scene2d.js

Line 474 in 8731cb5

this.glplot.draw();

does the same thing. 🔪 away!

:hocho -> 0ecaaaf

alexcjohnson · 2018-03-05T08:51:39Z

src/plots/gl2d/scene2d.js

-        if(bounds[i] > bounds[i + 2]) {
-            bounds[i] = -1;
-            bounds[i + 2] = 1;
-        }


Also seems like this.bounds has stopped being used, so I 🔪 it. Can't wait until we can 🔪 this whole subplot type...

etpinard · 2018-03-05T16:40:21Z

src/plots/cartesian/autorange.js

+    if(!ax._m) ax.setScale();
+
+    var len = data.length;
+    var extrapad = options.padded || false;


Correct me if I'm wrong, but this used to be the only place in expand() that used ax._length as:

plotly.js/src/plots/cartesian/axes.js

Line 455 in 8731cb5

extrappad = options.padded ? ax._length * 0.05 : 0,

this used to be the only place in expand() that used ax._length

Exactly - taking that out, and instead holding on to extrapad inside the ax._min/_max entries, to be evaluated during getAutorange, is what fixed #2428 and enabled calc->plot

Great! That makePadFn helper above is brilliant ✨ and expand() is much clearer and well commented 📚

Now, would mind mentioning in the jsDoc above that expand() should not depend on pixel values (e.g ax._length), but only on calcdata things?

fuller docs -> 9913afe

etpinard · 2018-03-05T17:06:37Z

src/plots/cartesian/autorange.js

+
+    var i, j, k, v, di, dmin, dmax, ppadiplus, ppadiminus, includeThis, vmin, vmax;
+
+    function getPad(item) {


We could maybe rename this function. getPad might be confused with the output of makePadFn

haha I noticed that too... I'll change it.

renamed -> 9913afe

etpinard · 2018-03-05T17:08:30Z

src/plots/cartesian/autorange.js

+    }
+}
+
+function needsAutorange(ax) {


It feels awkward to export this now that auto-range routines are out of axes.js.

Maybe we should move fastAxisExpand out of scattergl/index.js and into this file?

Or just bake the fastAxisExpand functionality into expand to be used automatically when the paddings are constant - then everyone wins! I think that should be possible... I'll try it.

then everyone wins!

That's an idea! But perhaps the TOO_MANY_POINTS constant should remain per trace type?

etpinard · 2018-03-05T17:16:47Z

src/plots/cartesian/layout_attributes.js

@@ -86,7 +86,7 @@ module.exports = {
        values: ['normal', 'tozero', 'nonnegative'],
        dflt: 'normal',
        role: 'style',
-        editType: 'calc',
+        editType: 'plot',


It would be nice to add a few tests like this one that 🔒 the new editType: 'plot'.

... as removing editType: 'calc' is a pretty be deal 🐎 we wouldn't want to regress from that in the future!

and use this instead of ad-hoc logic in scattergl and pointcloud

also fix a bug with relayout of 2D arrays

etpinard · 2018-03-05T23:01:54Z

src/traces/scattergl/index.js

    }
+    calcAxisExpansion(gd, trace, xa, ya, x, y, ppad);


Very clean. This is great!

To note though, this new combined pathway is little slower. Previously at 1e6 pts, fastAxisExpand would clock in at ~5ms for one coordinate, now we're more in a 20-30ms range. Even though this is significant, I think it's worthwhile to make scattergl autorange even more on-par with the rest of the library. 👍

The failing tests are in this new suite that got put in during #2404. Should be an easy fix.

Huh, let's unpack that performance issue. Previously the inner loop was:

for(var i = 0; i < vals.length; i += 2) { var v = vals[i]; if(v < b0) b0 = v; if(v > b1) b1 = v; }

this afternoon it was:

for(i = 0; i < data.length; i++) { v = data[i]; if(Math.abs(v) < FP_SAFE) { // data is not linearized yet so we still have to filter out negative logs if(v < vmin && (!isLog || v > 0)) vmin = v; if(v > vmax) vmax = v; } }

The old code was accidentally omitting half the points (i+= 2), so we should be comparing 10ms to 20-30 ms (unless the 20-30ms is for both coordinates?). Then I'm doing two extra things: 1) comparing to FP_SAFE so we don't get a broken graph from numbers very close to the limit of floating point numbers, and 2) ensuring positive numbers when on a log scale. The new code also has a bunch more logic before and after this loop, but nothing else scales with the data size, all that is presumably <1ms. In my own tests don't see the extra logic in the loop coming anywhere near doubling the time. What am I missing?

Anyway I noticed I could optimize it a bit by making two versions of the loop, one for log and one for other types. Seems worth a couple of extra lines since this gets so much traffic -> 247c626

Updated the failing test in cf48426

Great! You're latest patch in 247c626 got Axes.expand within 5ms of fastAxisExpand with i++ swapped in for i += 2 at 1e6 pts. Considering it's more robust and works with traces other than scattergl, I'll call this a win. 🥇

etpinard · 2018-03-05T23:03:05Z

test/jasmine/tests/plot_api_test.js

+                });
+
+                Plotly.relayout(gd, attr, axLayoutEdits[attr]);
+                expectReplot(attr);


💯 / 💯 test!

etpinard · 2018-03-06T15:17:21Z

test/jasmine/tests/gl2d_plot_interact_test.js

-                expect(gd._fullLayout.xaxis.range).toBeCloseToArray([-0.05, 1.05], 'x range');
-                expect(gd._fullLayout.yaxis.range).toBeCloseToArray([-0.11, 1.11], 'y range');
+                expect(gd._fullLayout.xaxis.range).toBeCloseToArray([-0.119, 1.119], 2, 'x range');
+                expect(gd._fullLayout.yaxis.range).toBeCloseToArray([-0.199, 1.199], 2, 'y range');


In comparison, at 1e5 - 1 pts (i.e. below the big-data threshold), we get:

@alexcjohnson I'll reference #2417 - but honestly feel free to close it if you think Axes.expand is satisfactory now.

Right, so I guess that result is saying that in this case the constant pixel padding we choose is a bit bigger than it would need to be... but this is a random case, a lot of real cases will have correlations, like all the biggest markers are near one edge, so I think this is a pretty good compromise.

so I think this is a pretty good compromise.

Yep. I think so too 👌

etpinard · 2018-03-06T15:24:39Z

Great bug fixing / perf improvement PR 💃 💃

alexcjohnson added 4 commits March 2, 2018 21:40

clean up config_test

85f0442

Fixes for both #2385 and #2428

9f774c0

update test images for #2428 fix

d9521e8

update gl2d images for #2428 fix

04806a2

alexcjohnson commented Mar 5, 2018

View reviewed changes

alexcjohnson mentioned this pull request Mar 5, 2018

[WIP] Vertically center multi-line labels on Y axis #2424

Merged

etpinard reviewed Mar 5, 2018

View reviewed changes

etpinard added bug something broken status: reviewable labels Mar 5, 2018

alexcjohnson added 5 commits March 5, 2018 13:14

🔪 commented-out code

0ecaaaf

clean up autorange.expand docs and var names

9913afe

Merge branch 'master' into react-autosize

f262dc9

fast logic for axis expand where only data is an array

3875fcc

and use this instead of ad-hoc logic in scattergl and pointcloud

test editType for attrs that affect axis length

8cf8d51

also fix a bug with relayout of 2D arrays

etpinard reviewed Mar 5, 2018

View reviewed changes

alexcjohnson added 2 commits March 5, 2018 21:42

update gl2d autorange test for converged autorange calculation

cf48426

optimize fast autorange further by splitting log into its own loop

247c626

etpinard reviewed Mar 6, 2018

View reviewed changes

alexcjohnson merged commit fa85e21 into master Mar 6, 2018

alexcjohnson deleted the react-autosize branch March 6, 2018 15:28

alexcjohnson mentioned this pull request Mar 6, 2018

Look for ways to improve big-data axis-expand algo #2417

Closed

nicolaskruchten mentioned this pull request Mar 7, 2018

Regression bugs in plotly since updating to 1.32.0 plotly/plotly.R#1211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autosize / autorange fixes #2437

Autosize / autorange fixes #2437

alexcjohnson commented Mar 5, 2018

alexcjohnson Mar 5, 2018

etpinard Mar 5, 2018

alexcjohnson Mar 6, 2018

alexcjohnson Mar 5, 2018

alexcjohnson Mar 5, 2018

etpinard Mar 5, 2018

alexcjohnson Mar 6, 2018

alexcjohnson Mar 5, 2018

etpinard Mar 5, 2018

alexcjohnson Mar 5, 2018

etpinard Mar 5, 2018

alexcjohnson Mar 6, 2018

etpinard Mar 5, 2018

alexcjohnson Mar 5, 2018

alexcjohnson Mar 6, 2018

etpinard Mar 5, 2018

alexcjohnson Mar 5, 2018

etpinard Mar 5, 2018

etpinard Mar 5, 2018 •

edited

Loading

etpinard Mar 5, 2018

etpinard Mar 5, 2018 •

edited

Loading

alexcjohnson Mar 6, 2018

etpinard Mar 6, 2018 •

edited

Loading

etpinard Mar 5, 2018

etpinard Mar 6, 2018

alexcjohnson Mar 6, 2018

etpinard Mar 6, 2018

etpinard commented Mar 6, 2018


		var i, j, k, v, di, dmin, dmax, ppadiplus, ppadiminus, includeThis, vmin, vmax;

		function getPad(item) {

Autosize / autorange fixes #2437

Autosize / autorange fixes #2437

Conversation

alexcjohnson commented Mar 5, 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 Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Mar 6, 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

etpinard commented Mar 6, 2018

etpinard Mar 5, 2018 •

edited

Loading

etpinard Mar 5, 2018 •

edited

Loading

etpinard Mar 6, 2018 •

edited

Loading