Skip to content

Multi-axis-type sploms #2899

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 13 commits into from
Aug 15, 2018
Merged

Multi-axis-type sploms #2899

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
dd563bb
consider only splom dimensions on axis during axis autotype
etpinard Aug 13, 2018
9e1869d
copy back axes categories on axes that skipped makeCalcdata,
etpinard Aug 13, 2018
9da5a1b
add splom mutli axis type mock
etpinard Aug 13, 2018
f3c73db
swap (x|y)a -> (x|y)id for clarity
etpinard Aug 14, 2018
b3d27bd
skip dims with conflicting axis types
etpinard Aug 14, 2018
9039a8e
keep track of visible dimensions using visibleDims list
etpinard Aug 14, 2018
ab7d9c6
bring handleTypeDefaults api on-par
etpinard Aug 14, 2018
39b71bb
add dimensions[i].axis.type
etpinard Aug 14, 2018
57ddadb
pass gd to basePlotModule.updateFx
etpinard Aug 15, 2018
15efc72
clear additional 'matrixTrace' use by regl-splom for selection
etpinard Aug 15, 2018
1823904
log info when skipping splom dim of conflicting axis types
etpinard Aug 15, 2018
390f292
add mock of splom with conflicting axis types
etpinard Aug 15, 2018
adb0004
use 'j' in value loop
etpinard Aug 15, 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
9 changes: 5 additions & 4 deletions src/traces/splom/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,17 @@ function calc(gd, trace) {
xa = AxisIDs.getFromId(gd, trace._diag[i][0]);
ya = AxisIDs.getFromId(gd, trace._diag[i][1]);

// if corresponding x & y axes don't have matching types, skip dim
if(xa && ya && xa.type !== ya.type) continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems likely to confuse people if they encounter this issue... do you want to include a Log message, or perhaps find a way to surface this in Plotly.validate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is part of the calc step (which is outside the scope of Plotly.validate) and that we can't easily move this to the defaults step (as axis defaults are coerced after trace defaults), I chose to only log something in 1823904

Note that I chose I was first planning on using Lib.warn instead of Lib.log, but currently the image server bails whenever a mock results in a Lib.warn, so the mock added in 390f292 would fail to generate an image. I hope you're ok with just a Lib.log here.


if(xa) {
makeCalcdata(xa, dim);
if(ya && ya.type === 'category') {
ya._categories = xa._categories.slice();
}
} else if(ya) {
} else {
// should not make it here, if both xa and ya undefined
makeCalcdata(ya, dim);
if(xa && xa.type === 'category') {
xa._categories = ya._categories.slice();
}
}
}
}
Expand Down
30 changes: 30 additions & 0 deletions test/jasmine/tests/splom_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,36 @@ describe('Test splom trace defaults:', function() {
});
});

describe('Test splom trace calc step:', function() {
var gd;

function _calc(opts, layout) {
gd = {};

gd.data = [Lib.extendFlat({type: 'splom'}, opts || {})];
gd.layout = layout || {};
supplyAllDefaults(gd);
Plots.doCalcdata(gd);
}

it('should skip dimensions with conflicting axis types', function() {
_calc({
dimensions: [{
values: [1, 2, 3]
}, {
values: [2, 1, 2]
}]
}, {
xaxis: {type: 'category'},
yaxis: {type: 'linear'}
});

var cd = gd.calcdata[0][0];

expect(cd.t._scene.matrixOptions.data).toBeCloseTo2DArray([[2, 1, 2]]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good, but I have various questions about what happens downstream from calc with the missing dimension(s)... like, cdata and ldata arrays don't match up with the dimension array anymore, does this get handled correctly in plotting? The next commit (visibleDims) seems like it assuages my concerns a bit, but would you mind including a mismatched-type dimension (somewhere in the middle of dimensions) in a mock so we can see it all the way to the end?

Copy link
Contributor Author

@etpinard etpinard Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Good call, done in -> 390f292

});
});

describe('@gl Test splom interactions:', function() {
var gd;

Expand Down