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 determineUnitForFormatting floating point error #6259

Merged
merged 2 commits into from
May 10, 2019

Conversation

benmccann
Copy link
Contributor

No description provided.

Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

I was also looking into this problem today.

src/scales/scale.time.js Outdated Show resolved Hide resolved
@simonbrunel
Copy link
Member

@benmccann can you please detail the issue (why we need +1 and the 0.1 epsilon) and ideally a codepen that reproduces it?

@nagix
Copy link
Contributor

nagix commented May 9, 2019

The reason of -1 is the number of ticks includes the ticks at start and end, but we want to divide (max - min) by the number of tick intervals.

@nagix
Copy link
Contributor

nagix commented May 9, 2019

The floating point error occurs when the time difference in milliseconds is converted to months or years because the length of a month or year varies.

@benmccann benmccann force-pushed the determineUnitForFormatting branch from ad8609f to b41969b Compare May 9, 2019 17:17
@benmccann
Copy link
Contributor Author

@simonbrunel I'm assuming you don't need me to create a codepen since you removed the "needs test case" label. The test I wrote reproduces it though and fails without this change

@simonbrunel
Copy link
Member

Correct, I was a bit confused about the - 1.1 but @nagix explanations helped.

@simonbrunel simonbrunel added this to the Version 2.9 milestone May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants