-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add grouping property to traces #1028
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
Conversation
After talking with @etpinard, I think a slightly more sophisticated/general approach is best. |
@etpinard @bpostlethwaite I guess this PR's relevance has been renewed! I'll check this over, but at a glance, looks like it implements |
adding |
Cool. I'll switch the name, extract the attrs, and add it across the board. |
|
||
if(trace.grouping) { | ||
if(trace.grouping[i] !== undefined && trace.grouping[i] !== null) { | ||
cd[i].grouping = String(trace.grouping[i]); |
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.
why do we need groupings
in gd.calcdata
?
just add it here: https://github.com/plotly/plotly.js/blob/master/src/plots/attributes.js all trace attributes are based off it - see here. |
@@ -67,7 +67,11 @@ module.exports = { | |||
}, | |||
ids: { | |||
valType: 'data_array', | |||
description: 'A list of keys for object constancy of data points during animation' | |||
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.
@etpinard is there a way to automatically cast these to strings, or do I have to manually loop through the array, if provided, and cast them?
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.
Oops. Meant to ask for groups, but same question applies to both.
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.
is there a way to automatically cast these to strings
Not at the moment. Casting within data_array
fields is done during the calc step at the moment.
As groups
will only be used in filter transforms at first, we could simplify cast groups
items in the filter calcTransform
handler.
Alternatively, we could add a block to Plots.doCalcdata
, casting groups
items once and for all. In order to behave correctly during restyle
and extendTraces
, this would (unfortunately) require adding groups
to the recalc-attribute list here.
I have no preference. Your call.
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 think it's best to leave unspecified… It could be country and you could filter by inclusion in a list, or it could be a value and you could filter numerically.
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.
As opposed to ids, which must be strings.
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 think it's best to leave unspecified…
very good point.
description: [ | ||
'An array of strings corresponding to each respective datum. These strings are not' + | ||
'inherently used by plotly for any purpose but may be used, for example, with transforms' + | ||
'in order to filter or group points by an auxilliary property.' |
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.
Lovely description. Thanks!
@etpinard Glad to make those changes, but can you clarify why |
Exactly. From #1028 (comment), if we would have chosen the cast-in- So, might pin down this functionality right now, if we chose to cast |
Obsolete. |
It's become clear that the
ids
field is insufficient. It's needed for object constancy while animating, but duplicateids
result in disappearing markers. This is the expected behavior; it just precludes overloading theids
field for something like a filter transform, where duplicate data is perfectly reasonable.This PR:
grouping
property to traces and explicitly notes that it has no inherent effectids
are invalid or at least have likely-unintended consequencescalcdata
tests for castingids
Feedback on naming and functionality is welcome. I don't like adding attributes, but this is very lightweight, seems necessary, and explicitly notes that it does not have any effect unless hooked up elsewhere by the user (e.g. transforms).
See also: #1027
cc: @chriddyp @etpinard @alexcjohnson