-
Notifications
You must be signed in to change notification settings - Fork 34
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
Decimal.round(1009680).toNumber() is 1009679.9999999999 #52
Comments
Apparently the first number this happens to is 800002 (Decimal.round(800002).toNumber() is 800001.9999999999). |
Specifically, this happens because
So we can't trust that an integer turned into mantissa*exp format and then having the reverse operation done remains an integer after a certain point. Cool. 1:53 PM] Patashu [10^^e308 EP]: I guess you could always do the same VeryNearlyRound() that add does |
floor and ceil are also busted for similar reasons |
ah, it's because I have a line like if (Math.abs(resultRounded - result) < ROUND_TOLERANCE) { and ROUND_TOLERANCE is 1e-10, and this is when it starts being hit |
yeah OK, now I remember why this is fundamentally tricky to fix. we can set the 'it looks close enough to an integer, we'll say it's an integer when we call toNumber()' threshold higher or lower than 1e-10, but it's fundamentally a problem of tradeoffs. as a workaround, you can round AFTER toNumber. but that won't fix one of floor/ceil sometimes being wrong, if you needed that. but, let me try 1e-9 and see how long it works for. EDIT: doesn't help for very long. I think this might just be one of those 'this is a limitation of the format, beware' things. going to close as 'fundamentally unsolvable; implement the fix you want yourself' |
One would expect that the result of Decimal.round would always be an integer (when converted to number), especially if the input was an integer, but apparently not. I'm not sure exactly what causes this or how to fix it.
The text was updated successfully, but these errors were encountered: