-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Introducing splom traces #2505
Introducing splom traces #2505
Conversation
- only markers for now, reuse scattergl convert logic for markers - use a splom basePlotModule to handle multiple traces and splom-wide field (like regl-line2d grids coming up)
which: - adds only a limited number svg layer in each <g .subplot> - use regl-line2d to draw grids
As of the commit fda1b34, things that work
things that do not work
✅ means works now I think. Here's what the |
src/plot_api/plot_schema.js
Outdated
@@ -486,7 +486,7 @@ function getLayoutAttributes() { | |||
|
|||
if(!_module.layoutAttributes) continue; | |||
|
|||
if(_module.name === 'cartesian') { | |||
if(_module.name === 'cartesian' || _module.name === 'splom') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this just based on _module.attr
?
if(Array.isArray(_module.attr)) { _module.attr.forEach(... handleBasePlotModule) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 9162265
src/traces/splom/attributes.js
Outdated
role: 'info', | ||
editType: 'plot', | ||
description: '' | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
layout.grid
doesn't have an analog of xdirection
, do we need that? And in place of ydirection
we have roworder: ('top to bottom'|'bottom to top')
which feels clearer to me than ('top'|'bottom')
. Actually I wonder if these belong in splom
attributes at all, or if we should just let layout.grid
handle it. It's nice for the simple single-trace case, but once you get two splom
traces, seems like only one (the first?) matters, so having one in each trace would be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I wonder if these belong in splom attributes at all, or if we should just let layout.grid handle it. It's nice for the simple single-trace case, but once you get two splom traces, seems like only one (the first?) matters, so having one in each trace would be confusing.
I agree 100% here. I'm thinking of 🔪 ing xdirection
and ydirection
. Users won't have a way to plot their splom dimensions from right to left until layout.grid.columnorder
is implemented, but I don't think that's a big deal. Any objections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔪 in a077c3f
src/traces/splom/attributes.js
Outdated
dflt: true, | ||
editType: 'plot', | ||
description: '' | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we will want the diagonals to support various types: scattergl, histogram, box, violin. So I'm wondering if instead of showdiagonal
we should have diagonal: {visible: (true|false)}
to support future expansion:
diagonal: {
visible: (true|false),
type: (scattergl|histogram|box|violin),
// then attributes specific to each type, as well as some overrides per dimension
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we will want the diagonals to support various types: scattergl, histogram, box, violin.
Interesting. So you're thinking a single splom trace could generate scatter panels AND histogram or box or violin traces on the diagonal? That would put us on-par with some of the seaborn. Yeah nice. I can't really think of any drawbacks from that approach except this might cause headaches to the workspace team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a single splom trace could generate scatter panels AND histogram or box or violin traces on the diagonal?
Yes exactly. An extra complication is that this would create even more x or y axes (for the histogram count or box/violin category), depending on the type and orientation of these diagonal plots; but once we do that, it seems like the implementation should be fairly straightforward. But anyway none of that has to happen now, as long as we set up the attributes we DO need now so they'll support this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @alexcjohnson, i'm really interested to use the diagonal type. Is it available to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the diagonal can only have scatter type. Follow #2555 for more info, but we do not have any immediate plans for when other types will be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh okay. Thanks
src/traces/splom/attributes.js
Outdated
dflt: true, | ||
editType: 'plot', | ||
description: '' | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we get rid of showdiagonal
I'm not sure it makes sense to turn these two into a little flaglist - perhaps just leave them as two booleans.
src/traces/splom/defaults.js
Outdated
var handleMarkerDefaults = require('../scatter/marker_defaults'); | ||
var handleLineDefaults = require('../scatter/line_defaults'); | ||
var PTS_LINESONLY = require('../scatter/constants').PTS_LINESONLY; | ||
var OPEN_RE = /-open/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, going to break for symbol numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> move to #2507 thanks!
src/traces/splom/defaults.js
Outdated
var xaxes = coerce('xaxes', xaxesDflt); | ||
var yaxes = coerce('yaxes', yaxesDflt); | ||
|
||
// TODO what to do when xaxes.length or yaxes.length !== dimLength ??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if dimLength
is longer it's easy to just drop the later dims. What if xaxes.length !== yaxes.length
or dimLength
is shorter than xaxes
or yaxes
? The easy thing to do is just truncate them all to the same length, and perhaps until we have a clear use case otherwise we should just do that.
Related though: if you have several splom
traces but they don't share all the same dimensions, you could of course handle this by specifying xaxes
and yaxes
for each, but could you also just pass in an empty dimension for ones you want to skip? If that takes more than 5 minutes to implement though, skip it for now and leave it as an item for future expansion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you already have dimensions[i].visible: false
, nice! That should take care of my "related" point, it'll still reserve x/y axes for these dimensions, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The easy thing to do is just truncate them all to the same length, and perhaps until we have a clear use case otherwise we should just do that.
That sounds right to me. I'll go with that unless anyone disagrees.
That should take care of my "related" point, it'll still reserve x/y axes for these dimensions, right?
Yes that should take care of it, but I haven't tried that yet. Putting that on my TODO list 📝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.... thanks for all these comments by the way. I hope you don't find the _hasOnlyLargeSplom
flag too hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcjohnson just to confirm what you meant in your related point:
Plotly.newPlot(gd, [{
type: 'splom',
dimensions: [{
values: [1, 2, 3]
}, {
values: [4, 5, 6]
}]
}, {
type: 'splom',
dimensions: [{
visible: false
}, {
visible: false,
}, {
values: [1, 2, 3]
}, {
values: [4, 5, 6]
}]
}])
gives:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 07d59d5
... the destroy method for regl-scattermatrix instance is broken, see gl-vis/regl-splom#3
test/image/mocks/splom_iris.json
Outdated
@@ -691,7 +691,6 @@ | |||
"layout": { | |||
"title": "Iris dataset splom", | |||
"width": 600, | |||
"height": 500, | |||
"showlegend": false | |||
"height": 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first cut baselines 🎉
cc @dfcreative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... notice how the legend items aren't showing the correct symbol. Fixing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore last comment. Something else fixed that.
Here's what happens with when allowing cross-subplot selections (like we have currently on Three things to notice:
With commits 71a0395 and 9d0bcfd we have a slightly more consistent (albeit incomplete) behavior: In brief, with these commits:
|
'geo', | ||
'pie', | ||
'ternary' | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great test!
with commit a79f5dc splom interactions are now 🔒ed , I'll tag this PR as reviewable. So, here's an updated pre-merge checklist:
Note that I just made two comments: arguing for leaving some previous unaddressed in this PR. |
As long as all the TODOs added here are linked to the upcoming splom open items issue(s) I agree, they're all fine to omit from this PR. Looking amazing guys! 💃 |
Here are some first-render benchmarks with
computed using: var Nvars = /* fill this in */
var Nrows = /* fill this in */
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.purge(gd);
console.time('splom')
Plotly.newPlot(gd, [{
type: 'splom',
dimensions: dims,
showupperhalf: false,
diagonal: {visible: false},
}], {width: 2000, height: 2000})
console.timeEnd('splom') |
Merging this thing! |
resolves #2372
This is a feature branch built off PRs #2474, #2487, #2492 and #2499 where all splom development should start off from. Other splom-related branches on
origin
likesplom
andsplom-dev
are now obsolete.cc @dfcreative @alexcjohnson