Skip to content
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

default diverging based on scheme #845

Merged
merged 4 commits into from
May 29, 2022
Merged

Conversation

mbostock
Copy link
Member

I was thinking of cases like this, where a diverging scheme is specified, but the scale type is not specified and it defaults to linear. It would be nice, when appropriate, to default to a diverging scale instead so that it is symmetric around zero.

https://observablehq.com/@violetr/30daychartchallenge-day-13-correlation

I did this by checking for whether the scheme is a diverging scheme, and whether a pivot is specified or whether the domain includes zero. The nice thing is that most of our tests that specify a diverging scale now merely need to specify a diverging scheme, though there were a couple tests that use a diverging scheme without a meaningful pivot (e.g., for temperature or IMDb rating where you want a diverging scheme simply for “low” and “high”).

@mbostock mbostock requested a review from Fil April 15, 2022 16:18
@mbostock
Copy link
Member Author

One thing I don’t like about this is that it will flip between a linear and a diverging scale based on whether the domain includes zero, so there could be an abrupt change based on the data. But if we don’t look at the data, anytime you want to use a diverging scheme with a linear scale, you’d need to specify the type as linear explicitly. Maybe that’s okay, or even good, because it forces you to think about it. Let me know if you prefer that.

@Fil
Copy link
Contributor

Fil commented May 26, 2022

Sorry for the belated answer. Yes, I'd prefer the second option—where the user has to explicitly indicate a type to use a diverging scheme on a non-diverging scale. Otherwise, the changes with the data extent will be too unexpected.

It will be a breaking change (which is fine), but I wonder if we should add a warning when none of the conditions are met. I think the list of conditions is: there is no (explicit) scale type, the pivot is undefined, the data is numeric (not temporal or ordinal), and the domain doesn't contain 0?

@mbostock mbostock force-pushed the mbostock/default-diverging branch from 4f6c817 to bf35645 Compare May 29, 2022 02:57
@mbostock
Copy link
Member Author

Can you take another look, @Fil? I’ve implemented your preferred option. I chose not to implement the warning because the warning is also dependent on the domain of the data and I think that could be noisy. (I’d like to know that as long as my data has the same shape as my test date, Plot will not generate a warning.)

If you think this is good, I can include this in the 0.5 release for initializers. Otherwise we probably should wait a while for 0.6 since this is nominally a breaking change.

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added documentation

@mbostock mbostock merged commit 913629f into main May 29, 2022
@mbostock mbostock deleted the mbostock/default-diverging branch May 29, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants