Skip to content

Splom zoom perf #2527

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

Merged
merged 14 commits into from
Apr 11, 2018
Merged

Splom zoom perf #2527

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
02ed2eb
move doAutoRangeAndConstraints, drawData and finalDraw to subroutines
etpinard Apr 5, 2018
3ad1eaa
add 'axrange' editType
etpinard Apr 5, 2018
bb02281
speed up doModeBar subroutines for cartesian subplots
etpinard Apr 5, 2018
d07ae70
improve fullLayout._has()
etpinard Apr 5, 2018
468119e
speed up dragbox
etpinard Apr 5, 2018
0979272
clean splom drag logic
etpinard Apr 5, 2018
ebb35ce
lint in plot_api
etpinard Apr 5, 2018
f9090b7
(fixup) add draggedPts to scattercarpet categories
etpinard Apr 6, 2018
b2ee736
Merge branch 'splom-feature' into splom-zoom-perf
etpinard Apr 9, 2018
a0b2574
Merge branch 'splom-feature' into splom-zoom-perf
etpinard Apr 9, 2018
d3fe40d
replace selectAll with for(k in _plots) loop
etpinard Apr 10, 2018
cc1b3de
fixup and :lock: dragbox with linked y axes
etpinard Apr 10, 2018
980855c
Merge branch 'splom-feature' into splom-zoom-perf
etpinard Apr 11, 2018
f7d637d
:books: link TODOs to gh issues
etpinard Apr 11, 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
3 changes: 2 additions & 1 deletion src/plot_api/edit_types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ var layoutOpts = {
valType: 'flaglist',
extras: ['none'],
flags: [
'calc', 'calcIfAutorange', 'plot', 'legend', 'ticks', 'margins',
'calc', 'calcIfAutorange', 'plot', 'legend', 'ticks', 'axrange', 'margins',
'layoutstyle', 'modebar', 'camera', 'arraydraw'
],
description: [
Expand All @@ -48,6 +48,7 @@ var layoutOpts = {
'*legend* only redraws the legend.',
'*ticks* only redraws axis ticks, labels, and gridlines.',
'*margins* recomputes ticklabel automargins.',
'*axrange* minimal sequence when updating axis ranges.',
'*layoutstyle* reapplies global and SVG cartesian axis styles.',
'*modebar* just updates the modebar.',
'*camera* just updates the camera settings for gl3d scenes.',
Expand Down
36 changes: 36 additions & 0 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1683,6 +1683,26 @@ exports.relayout = function relayout(gd, astr, val) {

if(flags.legend) seq.push(subroutines.doLegend);
if(flags.layoutstyle) seq.push(subroutines.layoutStyles);

if(flags.axrange) {
// N.B. leave as sequence of subroutines (for now) instead of
// subroutine of its own so that finalDraw always gets
// executed after drawData
seq.push(
// TODO
// no test fail when commenting out doAutoRangeAndConstraints,
// but I think we do need this (maybe just the enforce part?)
// Am I right?
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to -> #2540

subroutines.doAutoRangeAndConstraints,
// TODO
// can target specific axes,
// do not have to redraw all axes here
subroutines.doTicksRelayout,
subroutines.drawData,
subroutines.finalDraw
);
}

if(flags.ticks) seq.push(subroutines.doTicksRelayout);
if(flags.modebar) seq.push(subroutines.doModeBar);
if(flags.camera) seq.push(subroutines.doCamera);
Expand Down Expand Up @@ -2147,6 +2167,14 @@ exports.update = function update(gd, traceUpdate, layoutUpdate, _traces) {
if(restyleFlags.colorbars) seq.push(subroutines.doColorBars);
if(relayoutFlags.legend) seq.push(subroutines.doLegend);
if(relayoutFlags.layoutstyle) seq.push(subroutines.layoutStyles);
if(relayoutFlags.axrange) {
seq.push(
subroutines.doAutoRangeAndConstraints,
subroutines.doTicksRelayout,
subroutines.drawData,
subroutines.finalDraw
);
}
if(relayoutFlags.ticks) seq.push(subroutines.doTicksRelayout);
if(relayoutFlags.modebar) seq.push(subroutines.doModeBar);
if(relayoutFlags.camera) seq.push(subroutines.doCamera);
Expand Down Expand Up @@ -2299,6 +2327,14 @@ exports.react = function(gd, data, layout, config) {
if(restyleFlags.colorbars) seq.push(subroutines.doColorBars);
if(relayoutFlags.legend) seq.push(subroutines.doLegend);
if(relayoutFlags.layoutstyle) seq.push(subroutines.layoutStyles);
if(relayoutFlags.axrange) {
seq.push(
subroutines.doAutoRangeAndConstraints,
subroutines.doTicksRelayout,
subroutines.drawData,
subroutines.finalDraw
);
}
if(relayoutFlags.ticks) seq.push(subroutines.doTicksRelayout);
if(relayoutFlags.modebar) seq.push(subroutines.doModeBar);
if(relayoutFlags.camera) seq.push(subroutines.doCamera);
Expand Down
2 changes: 1 addition & 1 deletion src/plots/cartesian/axes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 || {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunate, but important as relinkPrivateKeys does relink empty objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... does not relink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does not relink empty objects.

}

tickSubplots.map(function(subplot) {
Expand Down
6 changes: 3 additions & 3 deletions src/plots/cartesian/layout_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,10 @@ module.exports = {
valType: 'info_array',
role: 'info',
items: [
{valType: 'any', editType: 'plot+margins', impliedEdits: {'^autorange': false}},
{valType: 'any', editType: 'plot+margins', impliedEdits: {'^autorange': false}}
{valType: 'any', editType: 'axrange+margins', impliedEdits: {'^autorange': false}},
{valType: 'any', editType: 'axrange+margins', impliedEdits: {'^autorange': false}}
],
editType: 'plot+margins',
editType: 'axrange+margins',
impliedEdits: {'autorange': false},
description: [
'Sets the range of this axis.',
Expand Down
45 changes: 44 additions & 1 deletion test/jasmine/tests/plot_api_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,10 @@ describe('Test plot api', function() {
'layoutStyles',
'doTicksRelayout',
'doModeBar',
'doCamera'
'doCamera',
'doAutoRangeAndConstraints',
'drawData',
'finalDraw'
];

var gd;
Expand Down Expand Up @@ -613,6 +616,46 @@ describe('Test plot api', function() {
expectReplot(attr);
}
});

it('should trigger minimal sequence for cartesian axis range updates', function() {
var seq = ['doAutoRangeAndConstraints', 'doTicksRelayout', 'drawData', 'finalDraw'];

function _assert(msg) {
expect(gd.calcdata).toBeDefined();
mockedMethods.forEach(function(m) {
expect(subroutines[m].calls.count()).toBe(
seq.indexOf(m) === -1 ? 0 : 1,
'# of ' + m + ' calls - ' + msg
);
});
}

var trace = {y: [1, 2, 1]};

var specs = [
['relayout', ['xaxis.range[0]', 0]],
['relayout', ['xaxis.range[1]', 3]],
['relayout', ['xaxis.range', [-1, 5]]],
['update', [{}, {'xaxis.range': [-1, 10]}]],
['react', [[trace], {xaxis: {range: [0, 1]}}]]
];

specs.forEach(function(s) {
// create 'real' div for Plotly.react to work
gd = createGraphDiv();
Plotly.plot(gd, [trace], {xaxis: {range: [1, 2]}});
mock(gd);

Plotly[s[0]](gd, s[1][0], s[1][1]);

_assert([
'Plotly.', s[0],
'(gd, ', JSON.stringify(s[1][0]), ', ', JSON.stringify(s[1][1]), ')'
].join(''));

destroyGraphDiv();
});
});
});

describe('Plotly.restyle subroutines switchboard', function() {
Expand Down