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

Returns NaN instead of correct result for large or small numbers #7

Closed
s100 opened this issue Oct 25, 2016 · 6 comments
Closed

Returns NaN instead of correct result for large or small numbers #7

s100 opened this issue Oct 25, 2016 · 6 comments

Comments

@s100
Copy link

s100 commented Oct 25, 2016

Large positive numbers:

> var roundTo = require("round-to");
undefined
> roundTo(1e14, 4);
100000000000000
> roundTo(1e15, 4);
1000000000000000
> roundTo(1e16, 4);
10000000000000000
> roundTo(1e17, 4);
NaN
> roundTo(1e18, 4);
NaN
> roundTo(12345678901234567890, -3)
12345678901234567000
> roundTo(123456789012345678901, -3)
123456789012345680000
> roundTo(1234567890123456789012, -3)
NaN

Negative numbers of large magnitude:

> roundTo(-1234567890123456789012, -3)
NaN

Numbers of small magnitude:

> roundTo(1/1234, 3)
0.001
> roundTo(1/12345, 3)
0
> roundTo(1/123456, 3)
0
> roundTo(1/1234567, 3)
NaN
@sindresorhus
Copy link
Owner

// @sebastiansandqvist

@sebastiansandqvist
Copy link
Contributor

sebastiansandqvist commented Oct 26, 2016

@sindresorhus The solution to this is probably to get rid of the concise syntax that currently makes this all work. The way the rounding works right now, when you plug in 1e16 with precision 4 this is what is actually evaluated:

Number(Math.round(1e16 + 'e4') + 'e-4');

It works because 1e16 is instantly converted to 10000000000000000.
That number + 'e4' just adds four zeros. However, it breaks on 17 because there comes a point you cannot just keep adding zeros. So in this case, 1e+21 cannot be represented by 1 with 21 zeros.

So while coercing the string '100000000000000000000e-4' to a number works, '1e+21e-4' is just NaN.

If you want to keep the code terse then maybe just add a disclaimer in the documentation. If you want it to be robust enough to handle very large and very small numbers (or extreme precisions), multiplication and division instead of relying on coercing strings to numbers multiple times would be better.

@sindresorhus
Copy link
Owner

I think it should be able to handle large numbers.

@sebastiansandqvist
Copy link
Contributor

sebastiansandqvist commented Mar 19, 2017

@sindresorhus Looking into this some more, it looks like there are limited options.

1. Use the naive rounding formula that I think most people use:

function round(n, precision) {
  var power = Math.pow(10, precision);
  return Math.round(n * power) / power;
}

This works for really large numbers and doesn't require converting back and forth between strings and numbers. But it fails when floating point arithmetic fails. For instance: round(1.005, 2) returns 1 instead of 1.01 because 1.005 * 100 === 100.49999999999999.

2. Use Number.toLocaleString

This is a rough implementation, but it works. If the precision is negative, then we need to figure out how many significant digits to use. I did this by getting the length of the integer portion of the number as a string and adding it to the given precision, but there might be a nicer way. If the precision is positive, then we can use the built-in maximumFractionDigits option. However, since the maximumSignificantDigits can only be a number from 1 to 21, some very large numbers will break this implementation.

function round(n, precision) {
  if (precision < 0) {
    const length = String(n).split('.')[0].length;
    return parseFloat(n.toLocaleString('en', { maximumSignificantDigits: length + precision, useGrouping: false }));
  }
  return parseFloat(n.toLocaleString('en', { maximumFractionDigits: precision, useGrouping: false }));
}

This only handles the round case, and doesn't round up or round down specifically.

3. Current implementation

Unless there is another alternative, maybe just include a note in the readme that says that numbers above X are not supported.

@sindresorhus
Copy link
Owner

Could we maybe do 1 only when the number is larger than the current implementation can handle? That way we get accuracy for the common-case while also supporting large numbers.

@lukechilds
Copy link
Sponsor

@sindresorhus This is causing the NaN bug in atomiclabs/hyperdex#119

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

No branches or pull requests

4 participants