Skip to content

Ensure axis autorange is computed after transform operations #1260

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 5 commits into from
Jan 4, 2017
Merged

Ensure axis autorange is computed after transform operations #1260

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0003e22
split doCalcdata operation into different loops:
etpinard Dec 19, 2016
30a7960
clear axis _min and _max after transform calc loop
etpinard Dec 19, 2016
775dbb5
transforms: clear _categories after transform calc loop
etpinard Jan 4, 2017
47356cd
Merge branch 'master' into transform-calc-loops
etpinard Jan 4, 2017
c9044fa
only clear ax field before regular loop when transforms are present
etpinard Jan 4, 2017
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
64 changes: 35 additions & 29 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1921,8 +1921,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
plots.doCalcdata = function(gd, traces) {
var axList = Plotly.Axes.list(gd),
fullData = gd._fullData,
fullLayout = gd._fullLayout,
i, j;
fullLayout = gd._fullLayout;

var trace, _module, i, j;

// XXX: Is this correct? Needs a closer look so that *some* traces can be recomputed without
// *all* needing doCalcdata:
Expand Down Expand Up @@ -1951,46 +1952,51 @@ plots.doCalcdata = function(gd, traces) {
axList[i]._categories = axList[i]._initialCategories.slice();
}

// If traces were specified and this trace was not included,
// then transfer it over from the old calcdata:
for(i = 0; i < fullData.length; i++) {
// If traces were specified and this trace was not included, then transfer it over from
// the old calcdata:
if(Array.isArray(traces) && traces.indexOf(i) === -1) {
calcdata[i] = oldCalcdata[i];
continue;
}

var trace = fullData[i],
cd = [];

// If traces were specified and this trace was not included, then transfer it over from
// the old calcdata:
if(Array.isArray(traces) && traces.indexOf(i) === -1) {
calcdata[i] = oldCalcdata[i];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this part isn't new here, but... it isn't used anywhere, right? Looks to me like it was already broken by transforms (calcdata index and trace index aren't the same) and will be broken MORE by this PR (any trace not included will be dropped from autorange) so I'd remove it until we decide to do it right AND use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @rreusser

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, is that a bug to be using that as an array index like that? If so, that may well be my fault. Unless I'm wrong, the intention was to reduce the work when animating. Use case: two traces. One contains 10MB of data, the other has two points. When animating the two points, it's best to avoid touching the 10MB trace. Perhaps we need to fix this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, makes a lot of sense to want to do that. I guess for autorange we just need to store the various _min and _max entries in a different way so we can selectively replace only the ones for the traces we're recalculating. And trace index, depending on how this gets used (it's not yet, right?) you just have to be careful about what you mean by index, before or after transforms potentially insert new traces.

continue;
}
}

var _module;
if(trace.visible === true) {
// transform loop
for(i = 0; i < fullData.length; i++) {
trace = fullData[i];

// call calcTransform method if any
if(trace.transforms) {
if(trace.visible === true && trace.transforms) {
_module = trace._module;

// we need one round of trace module calc before
// the calc transform to 'fill in' the categories list
// used for example in the data-to-coordinate method
_module = trace._module;
if(_module && _module.calc) _module.calc(gd, trace);
// we need one round of trace module calc before
// the calc transform to 'fill in' the categories list
// used for example in the data-to-coordinate method
if(_module && _module.calc) _module.calc(gd, trace);

for(j = 0; j < trace.transforms.length; j++) {
var transform = trace.transforms[j];
for(j = 0; j < trace.transforms.length; j++) {
var transform = trace.transforms[j];

_module = transformsRegistry[transform.type];
if(_module && _module.calcTransform) {
_module.calcTransform(gd, trace, transform);
}
_module = transformsRegistry[transform.type];
if(_module && _module.calcTransform) {
_module.calcTransform(gd, trace, transform);
}
}
}
}

// clear stuff ...
for(i = 0; i < axList.length; i++) {
axList[i]._min = [];
axList[i]._max = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to self: maybe we should clear _categories too to fix #1181 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 775dbb5

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool - note though that this only fixes the first part of 1181, not the variant of it mentioned in #1181 (comment)

}

// 'regular' loop
for(i = 0; i < fullData.length; i++) {
var cd = [];

trace = fullData[i];

if(trace.visible === true) {
_module = trace._module;
if(_module && _module.calc) cd = _module.calc(gd, trace);
}
Expand Down
11 changes: 11 additions & 0 deletions test/jasmine/tests/transform_filter_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
var assertDims = require('../assets/assert_dims');
var assertStyle = require('../assets/assert_style');
var customMatchers = require('../assets/custom_matchers');

describe('filter transforms defaults:', function() {

Expand Down Expand Up @@ -844,6 +845,10 @@ describe('filter transforms calc:', function() {
describe('filter transforms interactions', function() {
'use strict';

beforeAll(function() {
jasmine.addMatchers(customMatchers);
});

var mockData0 = [{
x: [-2, -1, -2, 0, 1, 2, 3],
y: [1, 2, 3, 1, 2, 3, 1],
Expand Down Expand Up @@ -898,6 +903,9 @@ describe('filter transforms interactions', function() {
assertUid(gd);
assertStyle(dims, ['rgb(255, 0, 0)'], [1]);

expect(gd._fullLayout.xaxis.range).toBeCloseToArray([0.87, 3.13]);
expect(gd._fullLayout.yaxis.range).toBeCloseToArray([0.85, 3.15]);

return Plotly.restyle(gd, 'marker.color', 'blue');
}).then(function() {
expect(gd._fullData[0].marker.color).toEqual('blue');
Expand All @@ -915,6 +923,9 @@ describe('filter transforms interactions', function() {
assertUid(gd);
assertStyle([1], ['rgb(255, 0, 0)'], [1]);

expect(gd._fullLayout.xaxis.range).toBeCloseToArray([2, 4]);
expect(gd._fullLayout.yaxis.range).toBeCloseToArray([0, 2]);

done();
});
});
Expand Down