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

Rounding precision #133

Closed
tvainika opened this issue Jul 3, 2018 · 7 comments
Closed

Rounding precision #133

tvainika opened this issue Jul 3, 2018 · 7 comments
Milestone

Comments

@tvainika
Copy link

tvainika commented Jul 3, 2018

I modified your unit test should default rounding when parsing to use var round1 = currency(1.2349) instead of current 1.234 and that caused rouding to break (not half-up).

@scurker scurker changed the title Rounding 1.2349 => 1.24 is wrong Rounding precision Aug 7, 2018
@scurker
Copy link
Owner

scurker commented Aug 7, 2018

Currently, the bulk of the rounding happens inside of parse and relies on Number.toFixed() for decimal rounding.

// Handle additional decimal for proper rounding
v = v.toFixed(1);

Part of the issue at the moment, is only half cents are taken into consideration in this calculation. So 1.2349 will get parsed internally as 123.4 which eventually rounds down to 123. Accounting for more decimals could potentially lead to a precision loss on higher numbers, but I need to do some testing around this to verify.

@charlie-s
Copy link
Contributor

charlie-s commented Sep 18, 2018

1.2349 appears to be getting parsed internally as 123.49, and then rounded to 124, due to Number(123.49).toFixed(1) rounding .49 to .5, and then the final round call turning 123.5 into 124.

Is there harm in using .toFixed(2)?

I'll give you my use case – 7.35% sales tax on $81.97, is resulting in $6.03 of sales tax when the actual amount is $6.02.

@scurker
Copy link
Owner

scurker commented Sep 24, 2018

It's more of an issue of agnostically handling any arbitrary precision. Using .toFixed(2) will solve a greater amount of cases, but it feels like it's not truly addressing the issue. It's definitely something that can make the next release, but I want to ensure there's a better scalable solution long term.

@charlie-s
Copy link
Contributor

I appreciate that immensely because as I was looking at the code and trying to think about this in higher and lower levels, I was coming to the same conclusion – I wish it could be solved in a more sound or proper way. If you want to discuss here or in a different issue, tag me. It's an interesting problem in general and I'm curious as to where it would land.

@scurker
Copy link
Owner

scurker commented Sep 24, 2018

Fortunately, toFixed(x) does not exhibit the same floating point issues since it's a string so even toFixed(100) would work and still be safe, so it's possible it could be a configurable option. But mostly I haven't been able to center on a good solution yet that resolves a ton of edge cases without bloating the library a whole bunch.

@popod
Copy link

popod commented Sep 25, 2018

What about this to round without errors ?

let roundedPrice = +(Math.round(price + 'e2') + 'e-2')

see: https://stackoverflow.com/a/19722641/8697449

@scurker scurker added this to the v1.2.0 milestone Sep 25, 2018
@scurker
Copy link
Owner

scurker commented Sep 26, 2018

I bumped up the internal precision slightly to account for errors. Should handle a greater number of cases within reasonable values.

@scurker scurker closed this as completed Sep 26, 2018
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

No branches or pull requests

4 participants