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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle big numbers when calculating nice range/numbers #107

Merged
merged 2 commits into from May 26, 2022

Conversation

bernardobelchior
Copy link
Contributor

Problem

Currently, graphic throws an error when calculating the step in linearNiceRange when using big numbers (e.g., 999999999999999999999999999999).

This happens because the step is being stored in an int which overflows and becomes negative.

In _tickIncrement we calculate the dart_math.log(step), which returns NaN because step is negative, which crashes the algorithm.

A similar thing happens in linearNiceNumbers.

Solution

By using a double instead of an int for storing the step, we are sure it never overflows. It may lose some precision, which I'd argue is ok, since this is a visualization tool and eyes can't see that precisely, but it doesn't throw an exception due to an overflow.

I'm open to other solutions as well, but this one seems to fix the aforementioned issue and does minimal impact.

Let me know what you think 馃槃

@bernardobelchior bernardobelchior marked this pull request as ready for review May 20, 2022 15:40
@bernardobelchior
Copy link
Contributor Author

Hey, @entronad! Let me know what you think about this 馃槃

@entronad
Copy link
Owner

Sorry missed this PR.

I believe your solution is proper, because this algorithm is from a JavaScript lib (AntV). we know that all numbers in JavaScript are doubles. I haven't noticed that 10 in Dart will limit pow to int scope.

Only one thing, I suggest, that 10.0 is better than 10.toDouble() to indicate a double. Const literals seem more concise and efficient.

@bernardobelchior
Copy link
Contributor Author

Updated 馃槃

@entronad entronad merged commit ad19428 into entronad:main May 26, 2022
@bernardobelchior bernardobelchior deleted the fix/big-numbers branch May 26, 2022 09:03
@bernardobelchior
Copy link
Contributor Author

Thanks, @entronad! 馃槃 Could we create a new release with this change?

@entronad
Copy link
Owner

I would wait a few days to see if there are other changes to collect in one release.

@entronad
Copy link
Owner

entronad commented Jun 7, 2022

Published in v0.10.5

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.

None yet

2 participants