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

refine auto bar mark determination #1801

Merged
merged 2 commits into from
Aug 12, 2023
Merged

refine auto bar mark determination #1801

merged 2 commits into from
Aug 12, 2023

Conversation

mbostock
Copy link
Member

Fixes #1800. The logic is quite complicated — but I think it helped to start by looking at the reducers, since that determines whether there is a bin or group transform.

@mbostock mbostock requested a review from tophtucker August 11, 2023 20:41
@mbostock
Copy link
Member Author

Ah, foo. Looks like I’ve broken the autoRectColorReducer test… I’ll need to investigate.

@mbostock
Copy link
Member Author

Fix was easy! I just needed to make colorReduce take priority over xReduce and yReduce.

Copy link
Contributor

@tophtucker tophtucker left a comment

Choose a reason for hiding this comment

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

Nice!! So I guess isOrdinalReduce can be effectively inlined because you made the reducer the top-level switch?

Nice that it no longer depends on xZero / yZero, given the intuition that "bar" implies zero-ness. (Maybe explicit zero-ness let us break ties between barX and barY? But I can't think of a case.)

Played around and didn‘t see anything unexpected.

Test seems good. None of our sample data seems to naturally hit this case. I feel like it’s more about small-N stuff. Like one revenue number for each of three business segments for each of the last five years.

@mbostock mbostock merged commit 2d92890 into main Aug 12, 2023
@mbostock mbostock deleted the mbostock/auto-bar branch August 12, 2023 05:46
Fil pushed a commit that referenced this pull request Aug 21, 2023
* refine auto bar mark determination

* add another test
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* refine auto bar mark determination

* add another test
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.

Auto: can it be made to make a timeseries bar chart?
2 participants