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

toFixed error #53

Closed
Sparticuz opened this issue Nov 2, 2012 · 10 comments
Closed

toFixed error #53

Sparticuz opened this issue Nov 2, 2012 · 10 comments

Comments

@Sparticuz
Copy link

I got this:

accounting.toFixed(35.175,2)
"35.17"

I should be getting 35.18, right?

@lxcodes
Copy link

lxcodes commented Nov 20, 2012

Got the same error in my program --

total_amount = sum_results.invoice + sum_results.invoice * (results.customer.sales_tax.tax_rate / 100)
adjusted_payment_amount = accounting.toFixed(sum_results.new_payment, 2)
adjusted_total_amount = accounting.toFixed(total_amount, 2)
console.log "Total Amount: #{total_amount}"
console.log "Adjusted New Payment Amount: #{adjusted_payment_amount}"
console.log "Adjusted Total Amount: #{adjusted_total_amount}"

yields

Total Amount: 35.245
Adjusted New Payment Amount: 35.25
Adjusted Total Amount: 35.24

I would have thought that this would have rounded as well in this case.

@Sparticuz
Copy link
Author

So, after some research, the problem is that some numbers can't be accurately described using Floating Points, which is what Javascript uses for Numbers.

If you take a look at your numbers and how the toFixed function works, this is what happens:
35.245*10^2 = 3524.4999999999995
Which obviously, doesn't round to 3525, which would then be divided by 100 to get back to 35.25

There seem to be two solutions; The first and probably preferred is to use a strong decimal variable type (http://jsfromhell.com/classes/bignumber). This wasn't acceptable in my case. So I had to turn to the (probably wrong) way of using what I guess is called an Epsilon Delta value. Basically, it's adding in an insignificant digit to the initial value to prevent the 999 repeating problem.

I won't guarantee that this will work for you and I don't think this should be merged in master, but here's what I'm doing just the same.

var toFixed = lib.toFixed = function(value, precision) {
        precision = checkPrecision(precision, lib.settings.number.precision);
        var power = Math.pow(10, precision);
        var epsilon = typeof arguments[2] != 'undefined' ? arguments[2].substr(0,1).toLowerCase() : 'u';

        // Determine rounding direction, multiply up by precision, round accurately, then divide and use native toFixed():
        if (epsilon == "u") {
            return (Math.round(lib.unformat(value + 0.00001) * power) / power).toFixed(precision);
        } else if (epsilon == "d") {
            return (Math.round(lib.unformat(value - 0.00001) * power) / power).toFixed(precision);
        } else {
            return (Math.round(lib.unformat(value) * power) / power).toFixed(precision);
        }
    };

It takes a third optional argument of 'epsilon', if it's "u" it will round up (and properly round your 35.245 to 35.25).
(My code in line 4 will always use "u" and add in the epsilon value. If you don't want that change 'u' to '')
If it's "d" it will always round down. (Sometimes the floating point goes high)
If the epsilon isn't specified it will round using the floating point math.

I think that adding in such an insignificant digit won't affect the actual rounding of the number, but I've only tested that my use-cases work correctly.

We probably need a bonafide mathematician in here to tell me that I'm wrong and my code should burn in hell, but hey...it works for me.

@lxcodes
Copy link

lxcodes commented Nov 26, 2012

I went back to something like Math.round( Math.round( number * Math.pow( 10, 2 + 1) ) / Math.pow( 10, 1) ) / Math.pow(10,2) in order to do the rounding. Probably just as bad and maybe worse than yours. In the next rendition we are doing on this project, I went back to keeping everything in cents instead of fuss with decimals -- generally easier.

@tombevers
Copy link

I'm experiencing the same: http://jsfiddle.net/XyaC6/
This should output 4.52 instead of 4.51

stevenbristol added a commit to stevenbristol/accounting.js that referenced this issue Mar 20, 2014
@Sparticuz Sparticuz mentioned this issue Jul 11, 2014
@vizcay
Copy link

vizcay commented Jul 31, 2015

the toFixed function is still broken right? it promises classical rounding but I get:

accounting.toFixed(158.605, 2) = 158.60
accounting.toFixed(259.605, 2) = 259.61

Thanks

@Sparticuz
Copy link
Author

It's not that it's 'broken', but this library doesn't use a strong decimal type because Javascript uses floating point arithmetic. So this is a constraint on the language, not the library. Take a look at my post above for more information and a potential workaround #53 (comment)

@vizcay
Copy link

vizcay commented Jul 31, 2015

I know about base 2 rounding problems etc, but the homepage claims:

"Implementation of toFixed() that treats floats more like decimal values than binary, fixing inconsistent precision rounding in JavaScript (where some .05 values round up, while others round down):"

So yes.. it's broken, because it fails to deliver what it promises.

stevenbristol added a commit to stevenbristol/accounting.js that referenced this issue Jul 31, 2015
@stevenbristol
Copy link

This was my fault. I fixed the bug, but only in the js, not in the min.js. I just updated my pull request (#75) to include the min.js and bumped the version number.

@vizcay
Copy link

vizcay commented Jul 31, 2015

I've downloaded the minified version from your branch and now is fixed!

Thanks @stevenbristol

wjcrowcroft added a commit that referenced this issue Mar 16, 2017
Updated toFixed method to handle all floating point errors.

Fixes #163, #159, #145, #111, #99, #97, #84, #53
@wjcrowcroft
Copy link
Member

Thanks, this is fixed by #164.

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

6 participants