Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autosize / autorange fixes #2437

Merged
merged 11 commits into from
Mar 6, 2018
+837 −651
Merged

Autosize / autorange fixes #2437

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
85f0442
clean up config_test
alexcjohnson Mar 1, 2018
9f774c0
Fixes for both #2385 and #2428
alexcjohnson Mar 3, 2018
d9521e8
update test images for #2428 fix
alexcjohnson Mar 5, 2018
04806a2
update gl2d images for #2428 fix
alexcjohnson Mar 5, 2018
0ecaaaf
:hocho: commented-out code
alexcjohnson Mar 5, 2018
9913afe
clean up autorange.expand docs and var names
alexcjohnson Mar 5, 2018
f262dc9
Merge branch 'master' into react-autosize
alexcjohnson Mar 5, 2018
3875fcc
fast logic for axis expand where only data is an array
alexcjohnson Mar 5, 2018
8cf8d51
test editType for attrs that affect axis length
alexcjohnson Mar 5, 2018
cf48426
update gl2d autorange test for converged autorange calculation
alexcjohnson Mar 6, 2018
247c626
optimize fast autorange further by splitting log into its own loop
alexcjohnson Mar 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 39 additions & 16 deletions src/plots/cartesian/autorange.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ module.exports = {
getAutoRange: getAutoRange,
makePadFn: makePadFn,
doAutoRange: doAutoRange,
needsAutorange: needsAutorange,
expand: expand
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Expand Down Expand Up @@ -229,24 +228,48 @@ function expand(ax, data, options) {
var len = data.length;
var extrapad = options.padded || false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fuller docs -> 9913afe

var tozero = options.tozero && (ax.type === 'linear' || ax.type === '-');
var isLog = (ax.type === 'log');

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

var hasArrayOption = false;

function makePadAccessor(item) {
if(Array.isArray(item)) {
hasArrayOption = true;
return function(i) { return Math.max(Number(item[i]||0), 0); };
}
else {
var v = Math.max(Number(item||0), 0);
return function() { return v; };
}
}

var ppadplus = makePadAccessor((ax._m > 0 ?
options.ppadplus : options.ppadminus) || options.ppad || 0),
ppadminus = makePadAccessor((ax._m > 0 ?
options.ppadminus : options.ppadplus) || options.ppad || 0),
vpadplus = makePadAccessor(options.vpadplus || options.vpad),
vpadminus = makePadAccessor(options.vpadminus || options.vpad);
options.ppadplus : options.ppadminus) || options.ppad || 0);
var ppadminus = makePadAccessor((ax._m > 0 ?
options.ppadminus : options.ppadplus) || options.ppad || 0);
var vpadplus = makePadAccessor(options.vpadplus || options.vpad);
var vpadminus = makePadAccessor(options.vpadminus || options.vpad);

if(!hasArrayOption) {
// with no arrays other than `data` we don't need to consider
// every point, only the extreme data points
vmin = Infinity;
vmax = -Infinity;

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;
}
}

data = [vmin, vmax];
len = 2;
}

function addItem(i) {
di = data[i];
Expand All @@ -259,7 +282,7 @@ function expand(ax, data, options) {
// more than an order of mag, clip it to one order. This is so
// we don't have non-positive errors or absurdly large lower
// range due to rounding errors
if(ax.type === 'log' && vmin < vmax / 10) vmin = vmax / 10;
if(isLog && vmin < vmax / 10) vmin = vmax / 10;

dmin = ax.c2l(vmin);
dmax = ax.c2l(vmax);
Expand All @@ -269,15 +292,6 @@ function expand(ax, data, options) {
dmax = Math.max(0, dmax);
}

// In order to stop overflow errors, don't consider points
// too close to the limits of js floating point
function goodNumber(v) {
return isNumeric(v) && Math.abs(v) < FP_SAFE;
}

function lessOrEqual(v0, v1) { return v0 <= v1; }
function greaterOrEqual(v0, v1) { return v0 >= v1; }

for(k = 0; k < 2; k++) {
var newVal = k ? dmax : dmin;
if(goodNumber(newVal)) {
Expand Down Expand Up @@ -329,3 +343,12 @@ function expand(ax, data, options) {
for(i = 0; i < 6; i++) addItem(i);
for(i = len - 1; i > 5; i--) addItem(i);
}

// In order to stop overflow errors, don't consider points
// too close to the limits of js floating point
function goodNumber(v) {
return isNumeric(v) && Math.abs(v) < FP_SAFE;
}

function lessOrEqual(v0, v1) { return v0 <= v1; }
function greaterOrEqual(v0, v1) { return v0 >= v1; }
17 changes: 3 additions & 14 deletions src/traces/pointcloud/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@
var createPointCloudRenderer = require('gl-pointcloud2d');

var str2RGBArray = require('../../lib/str2rgbarray');
var expandAxis = require('../../plots/cartesian/autorange').expand;
var getTraceColor = require('../scatter/get_trace_color');

var AXES = ['xaxis', 'yaxis'];

function Pointcloud(scene, uid) {
this.scene = scene;
this.uid = uid;
Expand Down Expand Up @@ -201,19 +200,9 @@ proto.updateFast = function(options) {

proto.expandAxesFast = function(bounds, markerSize) {
var pad = markerSize || 0.5;
var ax, min, max;

for(var i = 0; i < 2; i++) {
ax = this.scene[AXES[i]];

min = ax._min;
if(!min) min = [];
min.push({ val: bounds[i], pad: pad });

max = ax._max;
if(!max) max = [];
max.push({ val: bounds[i + 2], pad: pad });
}
expandAxis(this.scene.xaxis, [bounds[0], bounds[2]], {ppad: pad});
expandAxis(this.scene.yaxis, [bounds[1], bounds[3]], {ppad: pad});
};

proto.dispose = function() {
Expand Down
31 changes: 3 additions & 28 deletions src/traces/scattergl/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ var arrayRange = require('array-range');
var Registry = require('../../registry');
var Lib = require('../../lib');
var AxisIDs = require('../../plots/cartesian/axis_ids');
var needsAutorange = require('../../plots/cartesian/autorange').needsAutorange;
var Drawing = require('../../components/drawing');
var formatColor = require('../../lib/gl_format_color');

Expand Down Expand Up @@ -104,14 +103,10 @@ function calc(gd, trace) {
// and an average size for array marker.size inputs.
if(count < TOO_MANY_POINTS) {
ppad = calcMarkerSize(trace, count);
calcAxisExpansion(gd, trace, xa, ya, x, y, ppad);
} else {
if(markerOptions) {
ppad = 2 * (markerOptions.sizeAvg || Math.max(markerOptions.size, 3));
}
fastAxisExpand(xa, x, ppad);
fastAxisExpand(ya, y, ppad);
} else if(markerOptions) {
ppad = 2 * (markerOptions.sizeAvg || Math.max(markerOptions.size, 3));
}
calcAxisExpansion(gd, trace, xa, ya, x, y, ppad);
Copy link
Contributor

@etpinard etpinard Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@etpinard etpinard Mar 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🥇


// set flags to create scene renderers
if(options.fill && !scene.fill2d) scene.fill2d = true;
Expand Down Expand Up @@ -141,26 +136,6 @@ function calc(gd, trace) {
return [{x: false, y: false, t: stash, trace: trace}];
}

// Approximate Axes.expand results with speed
function fastAxisExpand(ax, vals, ppad) {
if(!needsAutorange(ax) || !vals) return;

var b0 = Infinity;
var b1 = -Infinity;

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

if(ax._min) ax._min = [];
ax._min.push({val: b0, pad: ppad});

if(ax._max) ax._max = [];
ax._max.push({val: b1, pad: ppad});
}

// create scene options
function sceneOptions(gd, subplot, trace, positions) {
var fullLayout = gd._fullLayout;
Expand Down
16 changes: 8 additions & 8 deletions test/jasmine/tests/gl2d_pointcloud_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,11 @@ describe('@gl pointcloud traces', function() {
var mock = plotData;
var scene2d;

var xBaselineMins = [{'val': 0, 'pad': 50}, {'val': 0, 'pad': 50}, {'val': 3, 'pad': 50}, {'val': 1, 'pad': 50}, {'val': 1, 'pad': 50}, {'val': 1, 'pad': 50}, {'val': 1, 'pad': 50}];
var xBaselineMaxes = [{'val': 9, 'pad': 50}, {'val': 9, 'pad': 50}, {'val': 6, 'pad': 50}, {'val': 9, 'pad': 50}, {'val': 9, 'pad': 50}, {'val': 9, 'pad': 50}, {'val': 9, 'pad': 50}];
var xBaselineMins = [{val: 0, pad: 50, extrapad: false}];
var xBaselineMaxes = [{val: 9, pad: 50, extrapad: false}];

var yBaselineMins = [{'val': 0, 'pad': 50}, {'val': 0, 'pad': 50}, {'val': 9, 'pad': 50}, {'val': 3, 'pad': 50}, {'val': 4, 'pad': 50}, {'val': 5, 'pad': 50}, {'val': 6, 'pad': 50}];
var yBaselineMaxes = [{'val': 9, 'pad': 50}, {'val': 9, 'pad': 50}, {'val': 9, 'pad': 50}, {'val': 3, 'pad': 50}, {'val': 4, 'pad': 50}, {'val': 5, 'pad': 50}, {'val': 6, 'pad': 50}];
var yBaselineMins = [{val: 0, pad: 50, extrapad: false}];
var yBaselineMaxes = [{val: 9, pad: 50, extrapad: false}];

Plotly.plot(gd, mock.data, mock.layout).then(function() {
scene2d = gd._fullLayout._plots.xy._scene2d;
Expand All @@ -185,8 +185,8 @@ describe('@gl pointcloud traces', function() {
return Plotly.relayout(gd, 'xaxis.range', [3, 6]);
}).then(function() {

expect(scene2d.xaxis._min).toEqual(xBaselineMins);
expect(scene2d.xaxis._max).toEqual(xBaselineMaxes);
expect(scene2d.xaxis._min).toEqual([]);
expect(scene2d.xaxis._max).toEqual([]);

return Plotly.relayout(gd, 'xaxis.autorange', true);
}).then(function() {
Expand All @@ -197,8 +197,8 @@ describe('@gl pointcloud traces', function() {
return Plotly.relayout(gd, 'yaxis.range', [8, 20]);
}).then(function() {

expect(scene2d.yaxis._min).toEqual(yBaselineMins);
expect(scene2d.yaxis._max).toEqual(yBaselineMaxes);
expect(scene2d.yaxis._min).toEqual([]);
expect(scene2d.yaxis._max).toEqual([]);

return Plotly.relayout(gd, 'yaxis.autorange', true);
}).then(function() {
Expand Down