-
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
Precompute scaled channels. #280
Conversation
I gave it a go as well and I think it's going to work well. We'll still need to pass the scales for some marks (axes, for example); I'm not sure we need to pass the unscaled channels, but it might be interesting for some user-defined marks to have access to the raw channel. |
src/plot.js
Outdated
return Object.fromEntries(Object.entries(scales).map(([name, {scale}]) => [name, scale])); | ||
return Object.fromEntries(Object.entries(scales).map(([name, scale]) => [name, nullsafe(scale)])); | ||
} | ||
|
||
// TODO https://github.com/d3/d3-scale/pull/241/files | ||
function nullsafe({type, scale}) { | ||
return type === "quantitative" | ||
? Object.assign(x => x === null ? NaN : scale(x), scale) | ||
: 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.
If we land d3/d3-scale#241 we can revert this part.
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 a hack (I don’t think we’d want to use a getter for this — I’m thinking instead there would need to be a phase after the scales are constructed where Plot computes the scaled channels), but it works for the letterFrequencyBar and penguinSex examples.
Also, mark rendering ends up being simpler to boot.
Fixes #52.