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.

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

Faster axis autorange + remove calcIfAutorange edit type #2823

Merged
merged 15 commits into from
Aug 2, 2018

Conversation

etpinard
Copy link
Contributor

resolves #2546 and brings cartesian axis autorange relayout calls (e.g. on-plot-area double-click or the "autoscale" mode bar button) to perform to a level comparable to axis range relayout calls (e.g zoom in/out).

In brief, cartesian axis.autorange is now declared as editType: 'axrange' and edit type 'calcIfAutorange' is gone.

As mentioned in #2546 (comment), adapting the update pathway ended up being a little trickier than I'd imagine (especially for annotations, see a8fb653). But I'm pretty please with the results: a lot of old delicate "find-autoranged-axes" logic in restyle, relayout and react is gone (see 138a84f)

However, there's one drawback that is important to mention. While this PR speeds up axis autorange relayouts, it does slow down:

  • first render for graphs with set axis ranges (as Axes.expand is no longer skipped), and
  • updates of old 'calcIfAutorange' attributes (that are now declared as editType: 'calc') on graphs with set ranges. The most significant (I think) is scatter marker.size where updating its values will now always lead to a calc edit pathway.

It would be nice to open a new edit pathway with a targeted calc step where only the relevant _module.calc / _module.setPositions or component calcAutorange would be called. But for the time being, I much prefer having fast double-click and mode bar interactions than fast marker.size restyle calls. I hope you agree.

This PR should be part of the next minor release 1.40.0, not a patch release in the 1.39.x series.

cc @alexcjohnson

_assert('set rng / no tickwidth', {
tickLen: 0,
pathd: 'M0,134.55H0M0,81V193.5M0,85.5H0'
});
Copy link
Contributor Author

@etpinard etpinard Jul 18, 2018

Choose a reason for hiding this comment

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

This assertion here fails on master, so count this as a bug fix.

@@ -1708,6 +1708,7 @@ function addAxRangeSequence(seq, rangesAltered) {
subroutines.doTicksRelayout;

seq.push(
subroutines.doAutoRangeAndConstraints,
Copy link
Contributor Author

@etpinard etpinard Jul 18, 2018

Choose a reason for hiding this comment

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

N.B. Adding doAutoRangeAndConstraints to the 'axrange' update sequence does work against some of the work done in #2628, but only by 1ms in the most extremes case (a 50x50 splom) when there are no axis constraints.

@etpinard
Copy link
Contributor Author

Some autoscale benchmarks for graphs that benefit the most.

using

Plotly.relayout(gd, { 'xaxis.range': [0, 2], 'yaxis.range': [0, 2] })
setTimeout(() => {
  console.time('autoscale')
  Plotly.relayout(gd, {
    'xaxis.autorange': true,
    'yaxis.autorange': true
  })
  console.timeEnd('autoscale')
}, 500)

Scattergl at 1e6 pts:

var N = 1e6
var x = []
var y = []
var ms = []

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

Plotly.newPlot(gd, [{
  type: 'scattergl',
  mode: 'markers',
  x: x,
  y: y,
  marker: {size: ms}
}], {
  dragmode: 'closest',
})
v1.39.2 this branch
900-1000 ms (no joke) 10-20 ms

Splom at 50 dims:

var Nvars = 50
var Nrows = 10
var dims = []
for(var i = 0; i < Nvars; i++) {
  dims.push({values: []})
  for(var j = 0; j < Nrows; j++) {
     dims[i].values.push(Math.random())
  }
}

Plotly.newPlot(gd, [{
  type: 'splom',
  dimensions: dims,
  showupperhalf: false,
  diagonal: {visible: false},
}], {width: 2000, height: 2000})
v1.39.2 this branch
1000-1200 ms ~250 ms

@@ -1858,24 +1858,21 @@ describe('Test axes', function() {
expect(ax._max).toEqual([{val: 6, pad: 10, extrapad: true}]);
});

it('should return early if no data is given', function() {
it('should fail if no data is given', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you're confident we always send an array to expand? I guess in principle if we don't have a required array the trace will be set visible: false...

Copy link
Contributor Author

@etpinard etpinard Jul 18, 2018

Choose a reason for hiding this comment

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

Yes, expand is only called from trace module calc or setPosition or component calcAutorange which are skipped for visible: false traces / component items.

I thought removing that if(!data) return; was a nice way to make sure expand is only called when needed.

@@ -96,8 +96,9 @@ function calcAxisExpansion(gd, trace, xa, ya, x, y, ppad) {
yOptions.padded = false;
}

Axes.expand(xa, x, xOptions);
Axes.expand(ya, y, yOptions);
// N.B. asymmetric splom traces call this with undefined xa or ya
Copy link
Collaborator

Choose a reason for hiding this comment

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

not undefined, that would error here as well as above... it's {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch -> 026cd53

@@ -19,7 +19,7 @@ module.exports = templatedArray('annotation', {
valType: 'boolean',
role: 'info',
dflt: true,
editType: 'calcIfAutorange+arraydraw',
editType: 'calc+arraydraw',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not for now of course, but annotations (showing, hiding, dragging in GUI) are a great argument in favor of each trace and item tracking its own autorange contributions, so it doesn't take a full recalc to move one annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so it doesn't take a full recalc to move one annotation.

Absolutely! I'll open an issue about this once this PR is merged.

_assert('auto rng / small tx', [-0.18, 3.035], [0.84, 3.365]);
})
.catch(failTest)
.then(done);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Beautiful test!


// save size in the annotation object for use by autoscale
options._w = annWidth;
options._h = annHeight;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, great to see all the pruning you manage to do in this PR!

@alexcjohnson
Copy link
Collaborator

@etpinard this is great, I have no qualms about this tradeoff and anyway we have a route to improve it even more later on with per-trace updates. A couple of minor comments but nothing blocking. 💃

@etpinard etpinard added this to the v1.40.0 milestone Jul 18, 2018
... this was used to not skip Axes.expand when ax autorange was false,
    but range slider range wasn't set.
@etpinard
Copy link
Contributor Author

etpinard commented Aug 2, 2018

Ok let's start merging things for 1.40.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faster axis autorange relayout
2 participants