Skip to content

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

Closed
wants to merge 3 commits into from
Closed

Conversation

rreusser
Copy link
Contributor

@rreusser rreusser commented Oct 11, 2016

It's become clear that the ids field is insufficient. It's needed for object constancy while animating, but duplicate ids result in disappearing markers. This is the expected behavior; it just precludes overloading the ids field for something like a filter transform, where duplicate data is perfectly reasonable.

This PR:

  • adds a grouping property to traces and explicitly notes that it has no inherent effect
  • notes in the docs that duplicate ids are invalid or at least have likely-unintended consequences
  • adds calcdata tests for casting ids

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

@rreusser
Copy link
Contributor Author

After talking with @etpinard, I think a slightly more sophisticated/general approach is best.

@rreusser
Copy link
Contributor Author

@etpinard @bpostlethwaite I guess this PR's relevance has been renewed! I'll check this over, but at a glance, looks like it implements grouping with tests, plot schema and everything……

@etpinard
Copy link
Contributor

adding groupings (I like groups better by the way) to all trace types would be even better.

@rreusser
Copy link
Contributor Author

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]);
Copy link
Contributor

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?

@etpinard
Copy link
Contributor

Cool. I'll switch the name, extract the attrs, and add it across the board.

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: [
Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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 opposed to ids, which must be strings.

Copy link
Contributor

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.

@etpinard etpinard added this to the v1.19.0 milestone Oct 18, 2016
@etpinard
Copy link
Contributor

etpinard commented Oct 18, 2016

TODO:

  • add filter calc transform here
  • add Plotly.restyle(gd, 'groups', [/* */]) making filter transforms with filtersrc: 'groups' work as expected here

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.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Lovely description. Thanks!

@rreusser
Copy link
Contributor Author

rreusser commented Oct 18, 2016

@etpinard Glad to make those changes, but can you clarify why Plotly.restyle(gd, 'groups', [...]) enters the picture? Is the goal to make sure that the transform updates the result when groups are modified?

@etpinard
Copy link
Contributor

etpinard commented Oct 18, 2016

Is the goal to make sure that the transform updates the result when groups are modified?

Exactly. From #1028 (comment), if we would have chosen the cast-in-Plots.doCalcdata option, this test case would effectively check that groups was including in the list of attribute to recalc on restyle.

So, might pin down this functionality right now, if we chose to cast groups items at some point.

@rreusser rreusser changed the title Add grouping property to scatter traces Add groups property to scatter traces Oct 19, 2016
@rreusser rreusser changed the title Add groups property to scatter traces Add grouping property to traces Oct 19, 2016
@etpinard
Copy link
Contributor

Obsolete.

@etpinard etpinard closed this Oct 20, 2016
@etpinard etpinard deleted the add-grouping-property branch November 15, 2016 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants