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

Multi-axis-type sploms #2899

Merged
merged 13 commits into from
Aug 15, 2018
Merged

Multi-axis-type sploms #2899

Show file tree
Hide file tree
Changes from 3 commits
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
3 changes: 2 additions & 1 deletion src/plots/cartesian/type_defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,10 @@ function setAutoType(ax, data) {
}
else if(d0.type === 'splom') {
var dimensions = d0.dimensions;
var diag = d0._diag;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recall that _diag is:

plotly.js/src/traces/splom/defaults.js

Lines 104 to 124 in def6aa5

// build list of [x,y] axis corresponding to each dimensions[i],
// very useful for passing options to regl-splom
var diag = traceOut._diag = new Array(dimLength);
// cases where showDiag and showLower or showUpper are false
// no special treatment as the xaxes and yaxes items no longer match
// the dimensions items 1-to-1
var xShift = !showDiag && !showLower ? -1 : 0;
var yShift = !showDiag && !showUpper ? -1 : 0;
for(i = 0; i < dimLength; i++) {
var dim = dimensions[i];
var xa = xaxes[i + xShift];
var ya = yaxes[i + yShift];
fillAxisStash(layout, xa, dim);
fillAxisStash(layout, ya, dim);
// note that some the entries here may be undefined
diag[i] = [xa, ya];
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah ok - a little confusing as usually xa is a full axis object, an id would be xId or something... but anyway nice fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cleaned up in -> f3c73db

for(i = 0; i < dimensions.length; i++) {
var dim = dimensions[i];
if(dim.visible) {
if(dim.visible && (diag[i][0] === id || diag[i][1] === id)) {
ax.type = autoType(dim.values, calendar);
break;
}
Expand Down
54 changes: 30 additions & 24 deletions src/traces/splom/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,39 @@ function calc(gd, trace) {
// only differ here for log axes, pass ldata to createMatrix as 'data'
var cdata = opts.cdata = [];
var ldata = opts.data = [];
var i, k, dim;
var i, k, dim, xa, ya;

function makeCalcdata(ax, dim) {
// call makeCalcdata with fake input
var ccol = ax.makeCalcdata({
v: dim.values,
vcalendar: trace.calendar
}, 'v');

for(var i = 0; i < ccol.length; i++) {
ccol[i] = ccol[i] === BADNUM ? NaN : ccol[i];
}
cdata.push(ccol);
ldata.push(ax.type === 'log' ? Lib.simpleMap(ccol, ax.c2l) : ccol);
}

for(i = 0; i < dimensions.length; i++) {
dim = dimensions[i];

if(dim.visible) {
var axId = trace._diag[i][0] || trace._diag[i][1];
var ax = AxisIDs.getFromId(gd, axId);
if(ax) {
var ccol = makeCalcdata(ax, trace, dim);
var lcol = ax.type === 'log' ? Lib.simpleMap(ccol, ax.c2l) : ccol;
cdata.push(ccol);
ldata.push(lcol);
xa = AxisIDs.getFromId(gd, trace._diag[i][0]);
ya = AxisIDs.getFromId(gd, trace._diag[i][1]);

if(xa) {
makeCalcdata(xa, dim);
if(ya && ya.type === 'category') {
ya._categories = xa._categories.slice();
}
} else if(ya) {
makeCalcdata(ya, dim);
if(xa && xa.type === 'category') {
xa._categories = ya._categories.slice();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

How could you get here? if xa exists you'll skip the else if(ya) block entirely.

But this raises an interesting (if possibly nonsensical) point - what if xa.type !== ya.type? I guess this could happen if one is numeric or date and the other is categorical, or if one is linear and the other is log. If the answer is "all hell breaks loose", which wouldn't surprise me, it may be appropriate to just log a warning and hide that dimension entirely. That seems like an abuse of the concept of a splom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How could you get here? if xa exists you'll skip the else if(ya) block entirely.

Good point. I wrote that up to preserve symmetry, but didn't bother checking. Thanks!

what if xa.type !== ya.type? I guess this could happen if one is numeric or date and the other is categorical, or if one is linear and the other is log.

I don't think this can happen on graphs with only 1 splom trace and no ax.type set in the layout. That's because (after this PR's 1st commit dd563bb), both x and y axes use the same data array in the axis autotype routine.

Scenarios where xa.type !== ya.type can happen if,

  • a trace before the splom trace in gd.data autotyped the axes
  • a users sets ax.type in the layout

which are things that don't happen by accident.

So, I yeah I think skipping that dimension entirely makes the most sense here. Thanks for bringing this up!

Copy link
Collaborator

Choose a reason for hiding this comment

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

a users sets ax.type in the layout
... [doesn't] happen by accident.

Hmm could happen in the editor though. @nicolaskruchten @VeraZab not sure how best to handle this. If we add dimension.type we should steer people there somehow when there's a splom on that axis. But if they still set an axis type directly would we want to find the corresponding opposite axis in the splom and set it to match, to avoid this error?

Copy link
Contributor

Choose a reason for hiding this comment

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

hoo boy. Well right now we don't support SPLOMs yet, and I'm thinking that when we do we should lock down axes/subplots quite tightly so that people can't break them too much :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skipping dims with axes of conflicting types in b3d27bd

}
}
}
Expand All @@ -63,8 +83,8 @@ function calc(gd, trace) {
dim = dimensions[i];

if(dim.visible) {
var xa = AxisIDs.getFromId(gd, trace._diag[i][0]) || {};
var ya = AxisIDs.getFromId(gd, trace._diag[i][1]) || {};
xa = AxisIDs.getFromId(gd, trace._diag[i][0]) || {};
ya = AxisIDs.getFromId(gd, trace._diag[i][1]) || {};

// Reuse SVG scatter axis expansion routine.
// For graphs with very large number of points and array marker.size,
Expand All @@ -91,20 +111,6 @@ function calc(gd, trace) {
return [{x: false, y: false, t: stash, trace: trace}];
}

function makeCalcdata(ax, trace, dim) {
// call makeCalcdata with fake input
var ccol = ax.makeCalcdata({
v: dim.values,
vcalendar: trace.calendar
}, 'v');

for(var i = 0; i < ccol.length; i++) {
ccol[i] = ccol[i] === BADNUM ? NaN : ccol[i];
}

return ccol;
}

function sceneUpdate(gd, stash) {
var scene = stash._scene;

Expand Down
Binary file added test/image/baselines/splom_multi-axis-type.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
45 changes: 45 additions & 0 deletions test/image/mocks/splom_multi-axis-type.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{
"data": [
{
"type": "splom",
"opacity": 0.9,
"showupperhalf": false,
"diagonal": {"visible": false },
"dimensions": [
{
"label": "numeric",
"values": [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
},
{
"label": "decimal",
"values": [-3.3, 2.2, -1.1, 0, 1.1, -2.2, 3.3, 4.4, -5, 6]
},
{
"label": "bool",
"values": [false, true, true, true, false, true, false, false, false, true]
},
{
"label": "0/1",
"values": [0, 1, 1, 1, 1, 1, 0, 0, 0, 0]
},
{
"label": "string",
"values": ["lyndon", "richard", "gerald", "jimmy", "ronald", "george", "bill", "georgeW", "barack", "donald"]
}
]
}
],
"layout": {
"hovermode": "closest",
"margin": {
"b": 80,
"l": 80,
"r": 30,
"t": 30
},
"yaxis2": {"type": "category"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This here might be tricky to find for users as e.g. nothing indicates that yaxis2 corresponds to the boolean data array.

Perhaps we could include this in the trace dimensions[i] containers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could include this in the trace dimensions[i] containers?

That's a great idea - dimension.type = 'category' to override the autotype for both axes. That would certainly reduce (though not eliminate) the chance of mismatches like #2899 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with dimensions[i].axis.type in 39b71bb

I'm thinking the extra container might make it cleaner if we add more "splom-generated-axis" settings e.g. I suspect some users would want to set dimensions[i].axis.showgrid: false

Copy link
Collaborator

Choose a reason for hiding this comment

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

haha exactly my rationale in #2899 (comment) - I should read all your comments before making my own. Anyway, love it.

"yaxis3": {"type": "category"},
"xaxis3": {"type": "category"},
"xaxis4": {"type": "category"}
}
}
6 changes: 4 additions & 2 deletions test/jasmine/tests/splom_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,10 @@ describe('Test splom trace defaults:', function() {
});

var fullLayout = gd._fullLayout;
expect(fullLayout.xaxis.type).toBe('date');
expect(fullLayout.yaxis.type).toBe('date');
expect(fullLayout.xaxis.type).toBe('linear', 'fallbacks to linear for visible:false traces');
expect(fullLayout.yaxis.type).toBe('linear', 'fallbacks to linear for visible:false traces');
expect(fullLayout.xaxis2.type).toBe('date');
expect(fullLayout.yaxis2.type).toBe('date');
});
});

Expand Down