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

fix: change binField to use min provided by bins #154

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

kwonoh
Copy link
Contributor

@kwonoh kwonoh commented Aug 17, 2023

If I understood binField and bins functions correctly, min provided by bins should be used for the output of binField since nice=true changes the min.

This PR replaces min to b.min

@domoritz domoritz changed the title Fix binField to use min provided by bins fix: change binField to use min provided by bins Aug 18, 2023
@kwonoh
Copy link
Contributor Author

kwonoh commented Aug 18, 2023

Repro: https://observablehq.com/d/137aafc8775f84fb

Screenshot 2023-08-18 at 4 36 43 PM

The min of values is 0.3 and the max of values is 9. However, the generated histogram looks like there is a value between 9.3 and 10.3.

I believe this is because of the below reason.

base and alpha in binField function are computed with b.min which is the "nicer" min returned by bins function.

However, the returning value of binField is computed with the original min.

Thus, I think the min in line 41 should be changed to b.min.

@jheer
Copy link
Member

jheer commented Aug 21, 2023

Good catch, thanks for the PR!

@jheer jheer merged commit 5b8a8b2 into uwdata:main Aug 21, 2023
2 checks passed
@kwonoh kwonoh deleted the kwonoh/fix-bin branch August 21, 2023 17:14
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