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

More consistent behavior of sqrt, nthRoot, and pow #851

Closed
balagge opened this Issue May 10, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@balagge

balagge commented May 10, 2017

Try:

a = nthRoot(i,3)

[{"r": 1, "phi": 0.5235987755983}, {"r": 1, "phi": 2.6179938779915}, {"r": 1, "phi": 4.7123889803847}]

The function returns array containing these objects instead of Complex numbers.

Also: for negative real radicands the degree of the root (currently) must be odd, so nthRoot(-1,2) throws exception. This is a bit weird, because sqrt(-1) returns i.

Even more weird and completely unintuitive: nthRoot(-1+0 i ,2) does not throw.

I think the problems are:

  • the implementation for complex vs real radicands should be consistent (currently for real radicands a single root is returned, for complex radicands all roots are returned).
  • the implementation of nthRoot() with degree of 2 should be consistent (identical) with the implementation of sqrt() (currently it is not).
  • the implementaion of nthRoot should be continuous @ real number radicands. so e.g. if x->(-3), then nthRoot(x,n) -> nthRoot(x,n), if possible.

These are hard to meet. I guess EITHER:

  • roots should only be implemented for real numbers (for complex numbers a power can be used), OR
  • roots of real numbers should return the principal OR all complex values as well.
@balagge

This comment has been minimized.

balagge commented May 10, 2017

or, since the "n'th root" operation cannot be fully reconciled for real vs complex radicands, there could be two separate functions for that. one function would compute the "real root" (and would allow real numbers only, and could return e.g. -2 for the cubic root of -8). The other would be the "complex root" and would return either the principal or all the roots.

But maybe this has been discussed elsewhere...

Anyway, the current implementation is inconsistent and returns garbage for complex radicands...

@balagge

This comment has been minimized.

balagge commented May 10, 2017

The principal issue of the return value is probably just a missing new Complex() constructor in the source, because the returned {r:... phi:...} objects are probably meant for the constructor call.

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented May 10, 2017

We've had lots of discussion about this in #486, and the behavior we decided on was to return the principal root (the root with the smallest angle, I think) for ^, and the real root for nthRoot, or when predictable is set:

(-8)^(1/3) = 1 + 1.7320508075689i (or -2 if predictable is set)
nthRoot(-8,3) = -2

I had forgotten about the odd behavior with complex radicands to nthRoot. It was mentioned in #486 but never resolved. Perhaps the complex case of nthRoot should be moved to a new function, nthRoots--using the proper complex number constructor this time. So nthRoots(x, n) returns n complex numbers in an array, where each element a in the array satisfies a^n = x. Since there could be someone relying on the strange behavior of nthRoot, this change might be a breaking change so it would be released in v4. Although, we could release nthRoots before removing the strange behavior from nthRoot.

So in summary:

pow(a, b) or a^b: Behavior is unchanged: returns the principal root, or the real root is predictable mode is set
nthRoot(x, n): Throws if x is complex, behavior is unchanged if x is real. Only returns the real root.
nthRoots(x, n): Returns n complex numbers, and works for real or complex x and integer n

Would this adequately address the issue?

@balagge

This comment has been minimized.

balagge commented May 11, 2017

For my part: yes, definitely. It resolves the confusing case if x is real and negative and n is odd, because

  • pow(x,1/n) will return the principal (complex) root (also configurable to return real root)
  • nthRoot(x,n) will return the real root.
  • nthRoots(x,n) will return everything
    so now I have perfect control of what I want to get

The only remaining case I am not really 100% confident about is the case if x is a negative real and n is even. nthRoot() currently throws exception. This is confusing because sqrt() does not throw but returns the principal complex square root. And intuitively sqrt(x) = nthRoot(x,2) should perhaps hold. So maybe nthRoot() should also return the principal root for any even n instead of throwing exception?

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented May 11, 2017

The only remaining case I am not really 100% confident about is the case if x is a negative real and n is even. nthRoot() currently throws exception.

We did this to match the behavior of Matlab. From https://www.mathworks.com/help/matlab/ref/nthroot.html:

Y = nthroot(X,N) returns the real nth root of the elements of X. Both X and N must be real scalars or arrays of the same size. If an element in X is negative, then the corresponding element in N must be an odd integer.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 11, 2017

👍 good summary of the points needed to get these functions better aligned.

@balagge

This comment has been minimized.

balagge commented May 12, 2017

We did this to match the behavior of Matlab.

Makes sense. Seems like Matlab also has a realsqrt() function, which is consistent with nthroot() (both work within the domain of real numbers only, both throw exception if negative real is used as argument). and a separate sqrt() which works on complex numbers.

I guess that should not be duplicated unless really necessary.

We can probably accept that sqrt(x) is NOT the same as nthRoot(x,2) after all.

@josdejong josdejong changed the title from Bug in nthRoot() function for complex radicands to More consistent behavior of sqrt, nthRoot, and pow Jun 4, 2017

@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 4, 2017

I've marked this issue as "help wanted". @ anyone please drop a message here if you're interested in picking this one up.

@dakotablair

This comment has been minimized.

Contributor

dakotablair commented Mar 7, 2018

I have submitted a patch for this (#1060), but let me know if there are any other changes you'd like to see.

@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 7, 2018

Thanks @dakotablair , I will look into it soon, hopefully coming weekend.

@josdejong josdejong referenced this issue Mar 24, 2018

Closed

Breaking changes for v5 #1045

7 of 7 tasks complete

josdejong added a commit that referenced this issue Mar 24, 2018

Merge pull request #1060 from dakotablair/develop
Fixed #851: More consistent behavior of sqrt, nthRoot, and pow

@josdejong josdejong closed this in c591c07 Jun 16, 2018

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