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

Format scientific notation returns NaN if negative exponential is beyond 7 points #512

Open
dtruong1801 opened this issue May 24, 2017 · 23 comments

Comments

@dtruong1801
Copy link

numeral return NaN in version 2.0.6 if format negative exponential is beyond 7 points.

numeral(1e-7).format('0,0.00')
version 2.0.4 numeral returns 0.00
version 2.0.6 numeral return NaN

@danawoodman
Copy link

Same here 😭

@tiesont
Copy link

tiesont commented Jun 30, 2017

For helping to troubleshoot: https://jsfiddle.net/86syrqL3/

Happens even with a exponential value produced by numeral itself. Probably worth doing a diff on the two versions to see what changed.

@bushmango
Copy link

We are also having this problem!

@JasonRammoray
Copy link

JasonRammoray commented Sep 7, 2017

The reason is hidden in the numeral.js, row 207:

int = numeral._.toFixed(value, 0, roundingFunction);

, which itself leads us to the implementation of toFixed method, defined in rows 380-406:

toFixed: function(value, maxDecimals, roundingFunction, optionals) {
            var splitValue = value.toString().split('.'),
                minDecimals = maxDecimals - (optionals || 0),
                boundedPrecision,
                optionalsRegExp,
                power,
                output;

            // Use the smallest precision value possible to avoid errors from floating point representation
            if (splitValue.length === 2) {
              boundedPrecision = Math.min(Math.max(splitValue[1].length, minDecimals), maxDecimals);
            } else {
              boundedPrecision = minDecimals;
            }

            power = Math.pow(10, boundedPrecision);

            // Multiply up by precision, round accurately, then divide and use native toFixed():
            output = (roundingFunction(value + 'e+' + boundedPrecision) / power).toFixed(boundedPrecision);

            if (optionals > maxDecimals - boundedPrecision) {
                optionalsRegExp = new RegExp('\\.?0{1,' + (optionals - (maxDecimals - boundedPrecision)) + '}$');
                output = output.replace(optionalsRegExp, '');
            }

            return output;
        }

Suppose we have a value equal to 1e-7 and we want to show it as an integer (this means, that we set maxDecimals to zero).
If we don't pass any optionals (and we don't pass any optionals at all in row 207), then minDecimals will be equal to zero, which means, that boundedPrecision will be also equal to zero, because splitValue is an array consiting of only one value (string '1e-7' in this case).
Then we calculate the value for power variable, which is equal to 1.
And then we hit the problem itself.
If we are using 1e-6, then this expression would give us '0':

output = (roundingFunction(value + 'e+' + boundedPrecision) / power).toFixed(boundedPrecision);

Which is in our case:

output = (Math.round(1e-6 + 'e+' + 0) / 1).toFixed(0) // gives '0'

But once we use 1e-7 we might be suprpised with the outcome, because

output = (Math.round(1e-7 + 'e+' + 0) / 1).toFixed(0) // gives 'NaN'

Actually it's pretty logic:

1e-7 + 'e+' + 0 === '1e-7e+0' // the string on the right hand side passed to Math.round results in NaN

@JasonRammoray
Copy link

JasonRammoray commented Sep 7, 2017

I see, that there is a fix already for that issue, but build on Travis has been failed.
That is why this pull-request is still opened.

@RaviDasari, any progress on that?

@RaviDasari
Copy link

@JasonRammoray , Yes. I fixed an extra comma in test case which is failing the build and it was green again. But I didn't see any updates from the authors. I am still waiting and I see so many PRs (from a very long time) in queue waiting to be merged by author.

@JasonRammoray
Copy link

JasonRammoray commented Sep 7, 2017

@RaviDasari , perhaps, it make sense to indicate authors who shall review your request.
I see, that "Reviewers" column is empty.
I recommend to add at least @adamwdraper as a primary reviewer.
From my side I'll also put a comment in your pr with a request to review it as soon as possible.

@RaviDasari
Copy link

@JasonRammoray , I am not able to add reviewers because of author settings! I see many PRs(all that I checked just now) don't have reviewers assigned. Perhaps only @adamwdraper can add reviewers to delegate work.

Nevertheless, @adamwdraper should be notified because he is the author and we mentioned him many times.

@JasonRammoray
Copy link

@RaviDasari, ok got it.
Let's wait for a while.

@michaelvolz
Copy link

Any news?

@RaviDasari
Copy link

RaviDasari commented Sep 21, 2017 via email

@turnerniles
Copy link

I'm not sure how we could get in touch with @adamwdraper to add more collaborators to the project but I tweeted him...

@RaviDasari
Copy link

RaviDasari commented Dec 4, 2017 via email

@artem-karnaukh
Copy link

Any news on this?

@turnerniles
Copy link

No news... It seems like either find another way to get in contact with @adamwdraper or find someone with who has the capacity to fork the repository and start a new repo / package.

@tab00
Copy link

tab00 commented Apr 11, 2018

Has @adamwdraper died, or been ignoring us?

What's the best thing to do now? I use the NPM package.

@JasonRammoray
Copy link

@tab00, I wouldn't make such guesses with respect to the author since anything could happen and we don't know that for sure.
I am not saying it is good to ignore all the issues this repository has, but lets focus on the productive ways of solving the issue.
There are several alternative solutions:

  1. roll back to the package version, which suits your project needs and makes all unit tests green
  2. [applicable if point 1) failed or you want to control the code by yourself] fork the repo, make changes, cover them with unit tests and fix all other tests in case if they will be broken. Then publish new npm package with similar name (you might also want to make it scoped)
  3. try to contact with the author @adamwdraper by any other means (phone, emails, messages in social networks)

@pqvst
Copy link

pqvst commented Aug 2, 2018

This issue has been bugging me for too long now. numbro seems to handle this fine, so I will probably switch to that instead.

brucehappy added a commit to brucehappy/Numeral-js that referenced this issue Sep 11, 2018
…tring is in exponential format).

Optimize toFixed when dealing with int values.
Removed some dead vars.

Related to adamwdraper#512, adamwdraper#543, adamwdraper#545, adamwdraper#596
@anton6
Copy link

anton6 commented Jan 28, 2019

Why is this issue still open? I am switching to "2.0.4" for now, but is this repo still active?

@SourceCipher
Copy link

This library should be marked as no longer active/maintained since the last release was in 2017 and all the bugs are ignored. I would recommend using another library which is still maintained.

@eidonjoe
Copy link

eidonjoe commented Sep 8, 2021

This library should be marked as no longer active/maintained since the last release was in 2017 and all the bugs are ignored. I would recommend using another library which is still maintained.

@SourceCipher Do you have any recommend?

@SourceCipher
Copy link

This library should be marked as no longer active/maintained since the last release was in 2017 and all the bugs are ignored. I would recommend using another library which is still maintained.

@SourceCipher Do you have any recommend?

We have replaced with the https://numbrojs.com library most of our code base

@eradman
Copy link

eradman commented Jan 26, 2024

@SourceCipher Do you have any recommend?

We have replaced with the https://numbrojs.com library most of our code base

Numbro also has a fatal flaw, namely rounding to integers does not work
BenjaminVanRyseghem/numbro#745

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