-
Notifications
You must be signed in to change notification settings - Fork 185
identity scale #305
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
identity scale #305
Conversation
test/plots/identity-scale.js
Outdated
|
||
export default async function() { | ||
return Plot.plot({ | ||
height: 396, |
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.
Setting the height is a little awkward. Currently setting the y-scale to an identity scale makes Plot think that there is no y-scale, and so you get a short plot by default (and no x and y axes). But there are still y channels here, so that logic’s a little off.
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.
Also there’s no way to turn the axes on if you have an identity scale, but I think in some cases you would want to show axes with an identity scale, even if it’s off by default.
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 wondered what the identity scale would mean for x and y. Passing the values untouched to render() mean we're talking about top-left corner of the frame (of the facet), with y pointing down. So we definitely have a y dimension and we could show an axis with domain = range = [0, frame 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.
I think we could use d3.scaleIdentity for x and y, maybe, closer to what you did in your PR. But I don’t think we should do additional type inference on the channel values if you’ve chosen an identity scale; it’d be reasonable to require that for x and y only because we know that pixel positions are quantitative. For anything else we don’t know what it represents.
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.
Okay, added that to the PR, which I think makes sense. You still get axes by default, but we could change that too if desired.
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.
Perfect! Including the demo that really hurts the eye, as a warning to not use this lightly :)
It works well with faceting, in the sense that the pixel origin ⟨0,0⟩ is the top-left corner of the facet’s frame.
Fixes #56. Supersedes #57.