-
Notifications
You must be signed in to change notification settings - Fork 185
Zero-dimensional grouping. #264
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
return typeof value === "undefined" || (value && value.toString === objectToString) ? value : {value}; | ||
return value === undefined || (value && | ||
value.toString === objectToString && | ||
typeof value.transform !== "function") ? value : {value}; |
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.
This is needed to detect Plot.identity as the default value. Related #262.
Like, I think if the group transform supported configurable outputs like the reduce transform currently does, rather than only being able to compute the frequency channel (L), then the group transform could subsume the reduce transform. Edit: Although, reducing code duplication isn’t particularly urgent. I think the real urgency is that the API feels consistent from the outside. |
I can see how
(But this can't be extended to normalize, I think?). Not sure if unifying means that groupX would become a shorthand for a more complete reduceX, or would share a similar API with {outputs}, {options}. Sorry this is not super helpful, I'm beginning to get a bit lost with all the new names. I think I'll need an index card for transforms, like the one for marks. |
|
Okay, I’m going to land this so I can resume working on documentation, but let me know if you have thoughts. |
The current list is convenient, but I wonder if it would not be more memorable if we did this list:
any non-default output would have to use an explicit {out: "r"}. Maybe less convenient though. |
@Fil If we did that, then I think we’d need to rename marks e.g. Plot.cell ↦ Plot.cellXY, Plot.dot ↦ Plot.dotXY. |
Ah, true. Here's another suggestion, where the capital letter only denotes the out channel:
This would mean switching groupX and groupY. In the examples:
(If I'm not mistaken) this approach seems more consistent (the letter denotes only the out channel instead of in/out; we use X throughout for horizontal bars), and introduces less symbols. I've created a branch to test it: #266 |
Fixes #261.
This introduces a new groupZ transform that groups only on the first of {z, fill, stroke}; it ignores x and y. The default output channel is fill, but convenience aliases groupZX, groupZY, and groupZR are provided to output to those respective channels.
The x channel is now optional to the groupX transform; if missing, it groups only on the first of {z, fill, stroke}, like groupZY. Same for the y channel for the groupY transform, and the x and y channels for the group transform.
The groupX transform now never groups on y and the groupY transform now never groups on x; use the group transform if this is desired.
For example, for a one-dimensional stacked bar chart:
There’s still a lot of overlap between the group transform and the reduce transform, which behaves differently. I think we should try to unify the code, and I think the grouping behavior of the reduce transform should probably behave like the group transform in this PR (meaning there probably should be a variant of reduce that groups on x and y, and a variant that groups on neither).