-
Notifications
You must be signed in to change notification settings - Fork 187
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
per-channel scale override, and “auto” scale #1247
Conversation
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’m maybe being too aggressive here by removing the checks on the scale domain—do you think we still want to default to the identity scale if the provided domain is colors or symbols? Feels like an edge case but may be worth doing for backwards-compatibility. Sadly we don’t have any tests for that behavior!
It will help users a lot, I think, in particular for the case you're showing where you want fill to be a scaled channel, and stroke to be either black or white. 🤔 However this might open to ambiguities in certain cases? Suppose you have two teams "team red" & "team cat". The first mark (or channel) represents both teams—so it will use a categorical color scale, but the second mark (or channel) only represents the "red" team, for some reason (say, a filter). Will team red be represented by the categorical color for the first mark, and the color "red" for the second mark? How might we let users opt-in (or out) explicitly? An alternative (thinking out loud) would be a helper Plot.color(x) which would return "something" that would always be scaled as x, independently of the color scale. Maybe a {color:"red"} object—or, the other way around, a {value: "red"} object which would always be passed through the scaled. |
Yes. I don’t think ambiguity in itself is a concern, but the problem here is that there is no way to resolve the ambiguity—there’s no way to disambiguate. In the current release of Plot you can do that by setting the type of the color scale (to identity if you want literal colors, or to linear or ordinal to encode abstract data; alternatively you can set a scheme or range). There’s no equivalent syntax in this PR for saying which one you want. I think we might want a way to override whether or not a channel is bound to a scale, or perhaps even which scale it is bound to. The latter might allow us to support dual-axis charts #147, too (not that I’m particularly excited about such a thing, but it might be sometimes useful). |
What if there were a scale option that let you specify the association of channel name with scale name? And, if you don’t specify what scale is associated with a given channel, the mark can specify a default (the current behavior). This way you can explicitly enable or disable a scale on a channel-by-channel basis. Plot.dot(
data,
Plot.stackY2({
x: (d) => 2021 - d.birth,
stroke: "gender",
fill: (d) => (d.gender === "F" ? "rgb(132, 165, 157)" : "#f6bd60"),
title: "full_name",
scale: {stroke: "color", fill: null}
})
) We could also support true (automatically chose the default scale for this channel) and false (equivalent to null). |
Sounds good! Very explicit. It would be nice also to opt out of projection (e.g. to position a label somewhere on the screen without having longitude/latitude coordinates). |
31df767
to
4f71fc0
Compare
The latest commit adds a mark scale option to override the association of channels and scales. I think there’s still some more work to do here, though…
|
The diff --git a/src/channel.js b/src/channel.js
index bce56f0c..dd6d2c9c 100644
--- a/src/channel.js
+++ b/src/channel.js
@@ -1,15 +1,26 @@
import {ascending, descending, rollup, sort} from "d3";
-import {first, isIterable, labelof, map, maybeValue, range, valueof} from "./options.js";
+import {first, isIterable, isObject, labelof, map, maybeValue, range, valueof} from "./options.js";
import {registry} from "./scales/index.js";
import {maybeReduce} from "./transforms/group.js";
// TODO Type coercion?
export function Channel(data, {scale, type, value, filter, hint}) {
+ const label = labelof(value);
+ if (isObject(value) && value.value !== undefined) {
+ if (value.transform != null) throw new Error(`the transform and value options are mutually exclusive`);
+ let scaleOption;
+ ({value, scale: scaleOption} = value);
+ if (scaleOption === false || scaleOption === null) {
+ scale = null;
+ } else if (scaleOption !== true) {
+ throw new Error(`unsupported scale option ${scale}`);
+ }
+ }
return {
scale,
type,
value: valueof(data, value),
- label: labelof(value),
+ label,
filter,
hint
}; |
@Fil that seems appealing, but I expect there are other places where we call Plot.valueof with a channel definition and we expect it to work. Those places will now need to check for a |
4f71fc0
to
de90f3f
Compare
de90f3f
to
a915648
Compare
Okay, I think this is good now. The fill, stroke, and symbol channels are now declared with the auto scale (rather than the color and symbol scales, respectively); this gives an indication that the scale should be inferred automatically based on the channel name and provided values, i.e. if literal values are provided, then no scale will be applied. Furthermore, you can now specify any channel as a {value, scale} object to indicate which scale should be bound to the channel, overriding the mark’s default. For example you can set the scale to null to opt-out of the scale, or set it to color to opt-in. |
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 seems that we should validate user input more strictly.
I've added a bit of documentation in #1269, I hope it helps but it's not super clear (which is why I put it in a different branch).
Also we'll need a few unit tests, and as I was fiddling with the hexbin-symbol.js test, I got errors (an Object appeared in the legend, and the symbols didn't respect the "value"). Maybe something doesn't work well with binning?
src/channel.js
Outdated
import {maybeReduce} from "./transforms/group.js"; | ||
|
||
// TODO Type coercion? | ||
export function Channel(data, {scale, type, value, filter, hint}) { | ||
export function Channel(data, {scale, type, value, filter, hint}, name) { | ||
let V = valueof(data, 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.
nit: we could have value = valueof(data, 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.
No; that would break the labelof(value)
below.
const {value} = channel; | ||
if (isOptions(value)) { | ||
channel = {...channel, value: value.value}; | ||
if (value.scale !== undefined) channel.scale = value.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.
Shouldn't we guard against bad scale values?
For example, we want to throw an error if the user passes
stroke: {value: "field", scale: "x"}
or
strokeOpacity: {value: Math.random, scale: "x"}
I was thinking we should test that channel.scale === "auto"
, and that value.scale is one of ["color", null] if name is stroke or color, and one of ["symbol", null] if name is symbol. And error in all other cases?
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’re saying we should restrict which scales we allow to be bound to which channels? Why? Is that likely? What if someone invents a creative use for doing something we didn’t anticipate?
I do think it’d be reasonable to enforce that the scale, if non-nullish, is a valid scale name (and not, say scale: "foo"
). But I’m not sure we should do any validation beyond 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.
Yes. I don't think the symbol or color channel can be assigned to x, even in a creative way—since their ranges are not in the same space? x, y, r and length are numbers, so there might be cases where you'd want to mix those, maybe—but most probably it's going to be a mistake, or a misunderstanding. (I also tried true, false… these will be fixed if we make sure that scale is nullish or a key of scales.)
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’ve added validation of the scale name when the channel is registered, but only that the scale name is known (in the scale registry). Note it was already the case that a mark could declare a channel bound to a scale that doesn’t exist; it was being treated as if the channel were not bound to a scale. Though since this PR offers users new control over how channels are bound to scales without implementing a custom mark, it seems reasonable to add some validation.
I understand that it doesn’t make sense to put scale: "x"
on the fill channel, or some such. But I also don’t see that we need to protect against it. There are so many other ways you can break plot. Adding validation requires us to codify which scales are allowed for which channels; we haven’t yet formalized this policy, and I don’t see a strong reason to do it now. Let’s address this in the future if users actually trip on it.
Co-authored-by: Mike Bostock <mbostock@gmail.com>
Here are two examples that create scales where they should opt out; possibly only due to the hexbin initializer (I'll dig more into it tomorrow). |
I’ve fixed the issues with initializers that you reported. Thanks @Fil! |
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.
To answer my own question, this also allows a mark to ignore a projection and give x, y as pixel values. You just need to opt out of both scales:
Plot.plot({
projection: "mercator",
marks: [
Plot.graticule(),
Plot.text(["hello"], {x: {value: [200], scale: null}, y: {value: [100], scale: null}})
]
})
This will be particularly useful for legends.
All that remains is the README.
* per-channel scale override; "auto" scale * document the {value, scale} pattern for color and symbol channels * Update README.md Co-authored-by: Mike Bostock <mbostock@gmail.com> * const style * validate scale name * fix inferred scale for initializers * inferChannelScale * less painful colors * Update README.md * Update README.md * Update README.md * Update README --------- Co-authored-by: Philippe Rivière <fil@rezo.net>
Fixes #1246. Consider this example from usCongressAgeColorExplicit where you want the stroke to go through the color scale, but for fill you want to specify a literal color:
Before:
After: