API,ENH: Change definition of complex sign

[mhvk]

This may be controversial, but the old definition of sign for complex numbers is really useless, and if we don't change it in 2.0, when will we? Note how the code in geomspace is substantially simplified by using the new definition!

From the release note included:

Following the API Array standard, the complex sign is now calculated as z / |z| (instead of the rather less logical case where the sign of the real part was taken, unless the real part was zero, in which case the sign of the imaginary part was returned). Like for real numbers, zero is returned if z==0.

EDIT: after discussion on the list, the consensus seemed to be that redefining sign was a good idea, but that the case for copysign was not strong. So, that is now removed from this PR.
With this, it has become possible to extend np.copysign(x1, x2) to complex numbers, since it can now generally return |x1| * sign(x2) with the sign as defined above (with no special treatment for zero).

[charris]

This needs a release note.

[charris]

And maybe a note on the mailing list.

[mhvk]

@charris - oops, had forgotten to include the changelog entry, now corrected (git commit -a doesn't work for new files, rightly, but I keep getting bitten by that...). I also sent a note to the mailing list (though it needs approval since it has been a /long/ time since I last posted...)

[mhvk]

note to reviewers: this was a small bug exposed by changing to an in-place multiplication with out_sign below.

[mtsokol]

As far as I'm able to review it, looks good to me! Could we have a small additional test_copysign_complex() next to test_sign_complex()?

[ngoldbaum]

I saw your email to the list and spent some time trying to find open source usages of np.sign that this would break. Long story short, I didn't see anything obvious, but it's hard to tell for sure. I did find a number of workarounds in other libraries (e.g. tensorflow has one).

It's not really something that this would break, but I did find that numba explicitly emulates numpy's behavior: https://github.com/numba/numba/blob/28e68e58c2268ac022d384b51417ab14d819814a/numba/np/npyfuncs.py#L579

So maybe someone should raise an issue with numba if this is merged?

[mhvk]

@mtsokol - thanks for the review. Asking for tests is always good - it smoked out some undesirable behaviour (e.g., real being inf while imag being nan). So, I now use the definition of sign properly inside copysign as well (taking care of the special case of zero).

[rgommers]

Thanks @mhvk! This does look like an improvement indeed. I think it also caught a gap in the array API test suite, or this would have shown up as a failure (maybe you can check this @mtsokol?).

I ran the SciPy test suite against this branch, and it showed a single error (I double checked, it's reproducible) in interpolate.interpn with the pchip method:

_________________________________ TestInterpN.test_complex[pchip] _________________________________
scipy/interpolate/tests/test_rgi.py:864: in test_complex
    assert_allclose(v1, v2)
        method     = 'pchip'
        points     = (array([0.5, 2. , 3. , 4. , 5.5, 6. ]), array([0.5, 2. , 3. , 4. , 5.5, 6. ]))
        sample     = array([[1. , 1. ],
       [2.3, 3.3],
       [5.3, 1.2],
       [0.5, 4. ],
       [3.3, 5. ],
       [1.2, 1. ],
       [3. , 3. ]])
        self       = <scipy.interpolate.tests.test_rgi.TestInterpN object at 0x7fc13b97b040>
        v1         = array([1.62962963-3.25925926j, 1.56572151-3.13144303j,
       1.77612073-3.55224146j, 2.        -4.j        ,
       1.18862863-2.37725726j, 1.62962963-3.25925926j,
       3.        -6.j        ])
        v2         = array([1.62962963-3.25925926j, 1.56572151-3.13144303j,
       1.77904436-3.55808873j, 2.        -4.j        ,
       1.18862863-2.37725726j, 1.62962963-3.25925926j,
       3.        -6.j        ])
        v2i        = array([-3.25925926, -3.13144303, -3.55808873, -4.        , -2.37725726,
       -3.25925926, -6.        ])
        v2r        = array([1.62962963, 1.56572151, 1.77904436, 2.        , 1.18862863,
       1.62962963, 3.        ])
        values     = array([[1.-2.j, 2.-4.j, 1.-2.j, 2.-4.j, 1.-2.j, 1.-2.j],
       [1.-2.j, 2.-4.j, 1.-2.j, 2.-4.j, 1.-2.j, 1.-2.j],
    ....j],
       [1.-2.j, 2.-4.j, 1.-2.j, 2.-4.j, 1.-2.j, 1.-2.j],
       [1.-2.j, 2.-4.j, 2.-4.j, 2.-4.j, 1.-2.j, 1.-2.j]])
        x          = array([0.5, 2. , 3. , 4. , 5.5, 6. ])
        y          = array([0.5, 2. , 3. , 4. , 5.5, 6. ])
/home/rgommers/mambaforge/envs/scipy-dev/lib/python3.10/contextlib.py:79: in inner
    return func(*args, **kwds)
E   AssertionError: 
E   Not equal to tolerance rtol=1e-07, atol=0
E   
E   Mismatched elements: 1 / 7 (14.3%)
E   Max absolute difference among violations: 0.00653744
E   Max relative difference among violations: 0.00164337
E    ACTUAL: array([1.62963 -3.259259j, 1.565722-3.131443j, 1.776121-3.552241j,
E          2.      -4.j      , 1.188629-2.377257j, 1.62963 -3.259259j,
E          3.      -6.j      ])
E    DESIRED: array([1.62963 -3.259259j, 1.565722-3.131443j, 1.779044-3.558089j,
E          2.      -4.j      , 1.188629-2.377257j, 1.62963 -3.259259j,
E          3.      -6.j      ])
        args       = (<function assert_allclose.<locals>.compare at 0x7fc1338632e0>, array([1.62962963-3.25925926j, 1.56572151-3.13144303j,...08873j, 2.        -4.j        ,
       1.18862863-2.37725726j, 1.62962963-3.25925926j,
       3.        -6.j        ]))
        func       = <function assert_array_compare at 0x7fc13f6ae5f0>
        kwds       = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=1e-07, atol=0', 'strict': False, ...}
        self       = <contextlib._GeneratorContextManager object at 0x7fc13f6a7340>

There are a handful of np.sign usages in the implementation:
https://github.com/scipy/scipy/blob/8cd805980852386324e18507a38380d18f3be4ef/scipy/interpolate/_cubic.py#L253-L289

It's hard to tell quickly what is going on there. It could be an unstable test - if the behavior of sign(0+x.j) changes and it's used for finding derivatives there, it may result in a slightly different answer.

(just seeing the new commit, will retest EDIT: indeed, no difference)

[mhvk]

@ngoldbaum - thanks for checking! And a good point about numba; I raised an issue there: numba/numba#9376

About scipy, my latest change is not likely to resolve the issue since it only deals with infinities and NaN. I'll try to see if I can figure out what is going on as well.

[mhvk]

p.s. Second push was just a linter issue, so no need to test that again! (EDIT: and third push an uncaptured invalid value error on FreeBSD).

[mhvk]

It is weird, the scipy function explicitly is for real values only according to its docstring. Maybe complex y and real x is OK, as I guess one can just interpolate real and imag separately... ... ... indeed, this is what is being tested.

To be honest, I think it is basically luck that that test passed with the existing definition of np.sign. I think this happened mostly because the test has values - 2j*values for the complex values, which guarantees that the derivatives change sign at the same location. ... testing ... yes, that is correct, after one also changes values to not just be a linear ramp (really!). Anyway, I'll raise an issue at scipy.

[mhvk]

See scipy/scipy#19739 for an issue about the failing complex interpolation (and how it can be made to fail also with the present definition of np.sign).

[ev-br]

Just to duplicate what is said on the scipy issue: the scipy test is wrong and it's safe to ignore it for this PR.

[mtsokol]

This does look like an improvement indeed. I think it also caught a gap in the array API test suite, or this would have shown up as a failure (maybe you can check this @mtsokol?).

@rgommers It looks like array-api-tests in test_sign uses Python's math.copysign as a reference implementation, which discards the imaginary part (I think it does something close to np.complex(...).astype(float)) and behaves as np.sign for complex currently.

Neither math nor cmath modules have a sign function so I think the only way is to provide a custom Python implementation as a reference here (use the new formula directly). I can update array-api-tests if it's Ok.

The array-api-tests CI stage doesn't fail here because test_sign lives in test_operators_and_elementwise_functions which is disabled due to the missing copy keyword in np.asarray.

One thing - I noticed that torch has a torch.sign which raises an error for the complex input and points to torch.sgn. I wonder when it would make sense to unify torch.sign and torch.sgn.

[rgommers]

I can update array-api-tests if it's Ok.

That'd be great, thanks!

One thing - I noticed that torch has a torch.sign which raises an error for the complex input and points to torch.sgn. I wonder when it would make sense to unify torch.sign and torch.sgn.

That indeed seems like an obvious improvement. And it should be backwards compatible. It's very well possible that torch.sign doesn't support complex because they wanted to avoid the behavior mismatch with numpy.sign in numpy 1.xx

[mhvk]

OK, following discussion on the mailing list, it seemed there was fair consensus on changing the definition of complex sign, but not on using it in copysign - so here I removed that (if somehow it turns out to be really wanted after all, I can put it back in a different PR).

Anyway, I think this is ready for final review...

[mtsokol]

I can update array-api-tests if it's Ok.

That'd be great, thanks!

FYI data-apis/array-api-tests#227 got merged today.

[mhvk]

@mtsokol approved. Does anyone else want to have a look? Or can this redefinition of sign for complex be merged?

[ngoldbaum]

Is it perhaps confusing that this is the case:

In [1]: np.sign(np.nan)
Out[1]: np.float64(nan)

In [2]: np.sign(complex(np.nan))
Out[2]: np.complex128(nan+nanj)

Perhaps if the real or imaginary part is equal to zero, we should return 0 instead of NaN for the real or imaginary part of the result.

[mhvk]

Not sure, my sense would be that when either component is NaN, it is best to propagate that to both; it also keeps the result closest to the definition of z/|z| (for |z}->0,±inf, one can argue about meaningful limits, but less so for NaN).

[ngoldbaum]

Thanks, on second thought I agree that having only a single kind of nan result here probably makes sense. I hope I never have a technical problem where complex NaN is something I need to worry about...

[ngoldbaum]

This got overwhelming support on the mailing list, let's pull this in. Thank you @mhvk!