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

Decimal.round(1009680).toNumber() is 1009679.9999999999 #52

Open
dan-simon opened this issue May 14, 2019 · 5 comments
Open

Decimal.round(1009680).toNumber() is 1009679.9999999999 #52

dan-simon opened this issue May 14, 2019 · 5 comments
Labels

Comments

@dan-simon
Copy link
Contributor

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.

@dan-simon
Copy link
Contributor Author

Apparently the first number this happens to is 800002 (Decimal.round(800002).toNumber() is 800001.9999999999).

@Patashu
Copy link
Owner

Patashu commented May 14, 2019

Specifically, this happens because

1.00968*1e6
1009679.9999999999

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
[1:53 PM] Patashu [10^^e308 EP]: (mult by 1e14, round, div by 1e14)

@Patashu Patashu added the bug label May 14, 2019
@Patashu
Copy link
Owner

Patashu commented Nov 21, 2021

floor and ceil are also busted for similar reasons
the root cause is toNumber though

@Patashu
Copy link
Owner

Patashu commented Nov 21, 2021

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

@Patashu
Copy link
Owner

Patashu commented Nov 21, 2021

yeah OK, now I remember why this is fundamentally tricky to fix.
basically, after we make our Decimal and it's split into mantissa and exponent, we don't know if it used to be an integer or very close to one. setting a 'this is meant to be an integer' flag is out of the question. (though ironically, break_eternity.js with its layer system has this for free!)

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'

Patashu added a commit that referenced this issue Nov 21, 2021
@Patashu Patashu added only if you're bored very difficult or of dubious value and removed only if you're bored very difficult or of dubious value labels Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants