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

accurateNumber() Causes Inaccurate Numbers #427

Closed
J-Griffin opened this Issue Mar 25, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@J-Griffin
Contributor

J-Griffin commented Mar 25, 2015

Because of javascript number handling quirks, the rounding done in the accurateNumber function can cause slightly bad values to come back. Note that this is probably the same problem as in Issue #385.

For a simplified example of this issue, run this in console

    // Expect 1.01, but get 1
    Number((Math.round(1.005*100)/100).toFixed(2));

This is happening to us for some values. The end effect is that we are prevented from reaching our "Max" value when the slider is at 100%.

A second effect is that some larger values actually lose fidelity even though they have no decimals at all. Try this:

    // Expect same number back, but actually get
    //             8301034833169289000  
    accurateNumber(8301034833169290000);

A way to fix this is to avoid using math to do the shifts.

    // Get 1.01 as expected
    Number(parseFloat(Math.round(1.005 + "e+2")+"e-2").toFixed(2));

Some stackoverflow references

A complete solution is pretty dirty, but looks something like this:

    // Rounds a number to 7 supported decimals.
    function accurateNumber(number) {
        var shiftedNumber = shiftNumber(number, 7);
        var roundedShiftedNumber = Math.round(shiftedNumber);
        var roundedNumber = shiftNumber(roundedShiftedNumber, -7);
        return Number(roundedNumber.toFixed(7));
    }
    // Shift numbers. Handle numbers even if they already have an exponent.
    function shiftNumber(number, places) {
        var splitNumber = number.toString().split("e");
        var numberPart = splitNumber[0];
        var currentExponent = splitNumber[1] ? parseInt(splitNumber[1]) : 0;
        var newExponent = currentExponent + places;
        return Number(numberPart + "e" + newExponent);
    };

Let me know what you think. I can submit a PR if you want to fix this.

@leongersen

This comment has been minimized.

Show comment
Hide comment
@leongersen

leongersen Mar 26, 2015

Owner

Ah, floating point. Always fun. Any reason we couldn't replace accurateNumber() by:

return Number(parseFloat(Math.round(number + "e+2")+"e-2").toFixed(2));
Owner

leongersen commented Mar 26, 2015

Ah, floating point. Always fun. Any reason we couldn't replace accurateNumber() by:

return Number(parseFloat(Math.round(number + "e+2")+"e-2").toFixed(2));
@J-Griffin

This comment has been minimized.

Show comment
Hide comment
@J-Griffin

J-Griffin Mar 26, 2015

Contributor

That mostly works, and probably a good idea if you want to avoid more complicated code. It does not work for very large numbers.

// returns 10.0000001 as expected
Number(parseFloat(Math.round(10.00000005 + "e+7")+"e-7").toFixed(7));

// Expect 100000000000005 or 1.00000000000005e+14
// Actual                    1.00000000000005e+21
Number(parseFloat(Math.round(100000000000005 + "e+7")+"e-7").toFixed(7));

This happens because shifting 7 places causes the number to always be represented with an exponent by toString(). So, the string "1.0e+21" + "e-7" == "1.0e+21e-7" which will not have the correct effect when re-parsed. Hence all the exponent parsing gymnastics in my original "solution".

Yes I agree, floating point is always fun. We've run into other issues like this several times in the past few months in our own code.

Contributor

J-Griffin commented Mar 26, 2015

That mostly works, and probably a good idea if you want to avoid more complicated code. It does not work for very large numbers.

// returns 10.0000001 as expected
Number(parseFloat(Math.round(10.00000005 + "e+7")+"e-7").toFixed(7));

// Expect 100000000000005 or 1.00000000000005e+14
// Actual                    1.00000000000005e+21
Number(parseFloat(Math.round(100000000000005 + "e+7")+"e-7").toFixed(7));

This happens because shifting 7 places causes the number to always be represented with an exponent by toString(). So, the string "1.0e+21" + "e-7" == "1.0e+21e-7" which will not have the correct effect when re-parsed. Hence all the exponent parsing gymnastics in my original "solution".

Yes I agree, floating point is always fun. We've run into other issues like this several times in the past few months in our own code.

@leongersen

This comment has been minimized.

Show comment
Hide comment
@leongersen

leongersen Jun 27, 2015

Owner

Could you help me setup a test case that demonstrates this issue?

Owner

leongersen commented Jun 27, 2015

Could you help me setup a test case that demonstrates this issue?

@J-Griffin

This comment has been minimized.

Show comment
Hide comment
@J-Griffin

J-Griffin Jul 1, 2015

Contributor

Sure, I should be able to write that sometime. Is calling directly against accurateNumber() acceptable, or would you like it to go through an entire end to end flow?

Contributor

J-Griffin commented Jul 1, 2015

Sure, I should be able to write that sometime. Is calling directly against accurateNumber() acceptable, or would you like it to go through an entire end to end flow?

@leongersen

This comment has been minimized.

Show comment
Hide comment
@leongersen

leongersen Jul 1, 2015

Owner

Ideally, I'd have a slider with a 'start' setting, and a get method call that returns the wrong value.

Owner

leongersen commented Jul 1, 2015

Ideally, I'd have a slider with a 'start' setting, and a get method call that returns the wrong value.

@leongersen leongersen removed this from the 8.0.0 milestone Jul 6, 2015

@utricularian

This comment has been minimized.

Show comment
Hide comment
@utricularian

utricularian Jul 14, 2015

Howdy @leongersen , why are we worried about the 7 digits of precision? Is there are reason to not let javascript play out the precision as far as it can? We're hitting problems with this code as well as its making it very difficult to guess where the percentages will be when the change event fires.

utricularian commented Jul 14, 2015

Howdy @leongersen , why are we worried about the 7 digits of precision? Is there are reason to not let javascript play out the precision as far as it can? We're hitting problems with this code as well as its making it very difficult to guess where the percentages will be when the change event fires.

@J-Griffin

This comment has been minimized.

Show comment
Hide comment
@J-Griffin

J-Griffin Aug 26, 2015

Contributor

I created pull request #504 with two new tests that demonstrate the issue.

Contributor

J-Griffin commented Aug 26, 2015

I created pull request #504 with two new tests that demonstrate the issue.

@leongersen leongersen added the Bug label Oct 9, 2015

@leongersen leongersen added the inDev label Oct 19, 2015

@leongersen

This comment has been minimized.

Show comment
Hide comment
@leongersen

leongersen Oct 25, 2015

Owner

I've added this example to the documentation discussing numbers that can't be stored as double precision floating points. I had some code in place that tried to work around it, but it got overly complicated. This lib gives an idea about working with arbitrary precision numbers.

Owner

leongersen commented Oct 25, 2015

I've added this example to the documentation discussing numbers that can't be stored as double precision floating points. I had some code in place that tried to work around it, but it got overly complicated. This lib gives an idea about working with arbitrary precision numbers.

@leongersen leongersen closed this Oct 25, 2015

@leongersen leongersen removed the inDev label Feb 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment