-
Notifications
You must be signed in to change notification settings - Fork 185
a generic alternative for the collapsed scales #474
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
apply min(range) to x1 and y1, and max(range) to x2 and y2. It concentrates the derogatory code in the application of the scale, instead of having to test it in each mark.
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 the PR and looking into consolidating the logic here!
I’ve not really articulated this as a principle before, but this is the first time we’re ascribing a special meaning to specific channels by name. Heretofore only scales had such special meanings, based on this registry:
Lines 18 to 29 in 2f326d2
// TODO Rather than hard-coding the list of known scale names, collect the names | |
// and categories for each plot specification, so that custom marks can register | |
// custom scales. | |
export const registry = new Map([ | |
["x", position], | |
["y", position], | |
["fx", position], | |
["fy", position], | |
["r", radius], | |
["color", color], | |
["opacity", opacity] | |
]); |
And eventually I thought we’d find a way to remove the specialness of scale names, although that seems less likely given the way Plot has grown, and certainly not urgent… though it could pave the way for marks to define new scales in the future.
So, I’d like to think about whether marks should declare their desire for this behavior explicitly (that is: whether to replace the channel value with an extremum in the case that the domain is collapsed rather than relying on the scale’s default behavior which is to use the midpoint of the range) rather than rely on a special behavior triggered by the channel name.
Overall, I think I’m inclined to proceed with my PR as it stands rather than trying to tackle the additional issues raised in this PR. Although mostly that’s because there’s a lot of other stuff I want to do. Always happy to discuss and appreciate the work you’ve put in here!
switch(name) { | ||
case "x1": | ||
case "y1": | ||
value = Array.from(value).fill(min(scale.range())); break; |
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 slightly different behavior than my PR this it will apply the scale’s inset. That might be desirable, though it’s different than the behavior of one-dimensional marks, and it’s also now dependent on the way the scale range is defined (which can be overridden). I assume it was done because the dimensions aren’t passed into this method, but it’s worth thinking about.
Also this requires materializing another array which is slow although not likely to be an issue. 😅
// domain is degenerate and represents only a single value such as [3, 3]; for | ||
// example, a rect will span the full extent of the chart along a collapsed | ||
// dimension (whereas a dot will simply be drawn in the center). | ||
export function isCollapsed(scale) { |
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.
You moved this into quantitative.js, but this behavior isn’t limited to quantitative scales (although that is where it most often applies). See the ordinalBar example, which should also be considered collapsed if the domain only contains a single 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.
Can you clarify the ordinalBar example? I've tried several variations and all seem to give the same chart under both branches. For example Plot.barY("A", {x: (d, i) => i, y: d => d}).plot()
gives a large rect (width x height).
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 example is with a point scale:
Plot.plot({
y: {
type: "point",
grid: true
},
marks: [
Plot.barY([0, 0, 0], {x: (d, i) => i, y2: d => d}),
Plot.ruleY([0])
]
})
This is admittedly a pretty pointless plot, but it does demonstrate the issue: in this PR, the y-domain is not considered collapsed because it is not a quantitative scale, even though its domain only has a single value [0].
Whereas in my PR you get:
@@ -179,6 +179,7 @@ export function ScaleQ(key, scale, channels, { | |||
|
|||
if (range !== undefined) scale.range(range); | |||
if (clamp) scale.clamp(clamp); | |||
scale.isCollapsed = isCollapsed(scale); |
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.
It’d be good to avoid the mutation here.
I wanted to see if there was a way to not complicate the marks' code for what is only a corner case. I attacked the scale, but maybe another approach would allow us to switch a mark with a collapsed dimension to a 1-d mark? 🤔 |
2ef9aa4
to
54afea0
Compare
Might revisit later. The "pros" of this branch were essentially in the separation of concerns (collapsed domains handled in the scale rather than in the marks, resulting in a code that seemed simpler to maintain). The "cons" are listed in the conversation :) |
apply min(range) to x1 and y1, and max(range) to x2 and y2.
It concentrates the derogatory code in the application of the scale, instead of having to test it in each mark.