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

Possible issue with the nthRoot() function #490

Closed
sonnyk22 opened this Issue Oct 26, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@sonnyk22

sonnyk22 commented Oct 26, 2015

After passing the expressions below to the MathJS.eval() function, I get the exception shown below thrown back.

nthRoot(e ^ -2, e ^ -4):
calc2

nthRoot(-2, 3)
calc3

However, on other calculators such as Microsoft, I do get results back.
calc1
calc4
calc6

Is the expression that I am passing in to the MathJS.eval() function incorrect?

@josdejong

This comment has been minimized.

Owner

josdejong commented Oct 28, 2015

Thanks for reporting this. I've done a little bit of debugging and found that JavaScripts Math.pow (used in an iteration in nthroot) doesn't always behave like we want. From MDN:

// due to "even" and "odd" roots laying close to each other, 
// and limits in the floating number precision, 
// negative bases with fractional exponents always return NaN
Math.pow(-7, 1/3); // NaN

In this particular case the algorithm evaluates the following, resulting in NaN:

Math.pow(-46.209093934213584, -0.9816843611112658); // NaN

So we have to see if we can improve nthRoot and work around this limitation in Math.pow.

@josdejong josdejong added the bug label Oct 28, 2015

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Oct 28, 2015

I will take a look at it tonight as well.

@josdejong

This comment has been minimized.

Owner

josdejong commented Oct 28, 2015

Ok thanks @ericman314 , I don't have time tonight so that would be great...

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Oct 29, 2015

See #496.

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Oct 29, 2015

I submitted a pull request #496 which replaces the algorithm with this:

// x = a ^ (1/root)
var x = Math.pow(Math.abs(a), 1/root);
// If a is negative and root is odd, then  (-1) ^ (1/root) = -1
x = a < 0 ? -x : x;

The square root algorithm (and cube root, etc.) is one of my favorite algorithms. The algorithm comes from Newton's method, which converges quickly but occasionally runs into problems. nthRoot(-2, 3) just happens to result in a division by zero, although choosing an alternative starting point could fix that.

I thought that perhaps the algorithm may be faster than my suggestion above when root is an integer, since pow can be optimized for integer exponents, but the timing results below are not looking very good for the algorithm even with the (supposed) optimization:

80000 iterations of nthRoot(x, y), real-valued y
original: 6483ms
proposed: 15ms

80000 iterations of nthRoot(x, y), integer-valued y
original: 1186ms
original-optimized: 1257ms
proposed: 12ms

Even when using my own "optimized" version of pow(real, integer) (which is not actually any faster than the built-in implementation, no surprises there!) the algorithm is slower. It looks like the proposed solution is much faster in every case.

@josdejong

This comment has been minimized.

Owner

josdejong commented Oct 30, 2015

This is fixed now in v2.4.1

@josdejong josdejong closed this Oct 30, 2015

@sonnyk22

This comment has been minimized.

sonnyk22 commented Oct 30, 2015

Thank you

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