MAINT: Deprecate Complex Ordering comparisons #17030
Conversation
| if (PyArray_ISCOMPLEX(operands[0]) && | ||
| PyArray_ISCOMPLEX(operands[1]) &&( |
There was a problem hiding this comment.
This should apply if even one of the operands is complex.
There was a problem hiding this comment.
I agree, will make the change.
| return -1; | ||
| } | ||
| const char *ufunc_name = ufunc_get_name_cstr(ufunc); | ||
| if (PyArray_ISCOMPLEX(operands[0]) && |
There was a problem hiding this comment.
What about the other operand here?
There was a problem hiding this comment.
This is operation on a single ndarray, hence we do not need check more i assume.
There was a problem hiding this comment.
Actually, it's not. You can see the signature for fmax, fmin, minimum and maximum here: https://numpy.org/doc/stable/reference/ufuncs.html#comparison-functions
All of them also have x2. Though I do realize why this can be confusing: np.max and np.min only take a single array. However, np.max is (roughly) np.maximum.reduce, and you can find the definition for ufunc.reduce here: https://numpy.org/doc/stable/reference/generated/numpy.ufunc.reduce.html#numpy.ufunc.reduce
There was a problem hiding this comment.
The uniform type resolvers output are uniform types though. So you may be able to only check one of them if you move this to the end of the function.
In fact, I may suggest creating a PyUFunc_SimpleUniformOperationTypeResolverDeprecateComplex which just calls the original version and then does this check. I am not a big fan of the strcmp here, and its not necessary (you will have to change type resolver in the ufunc API creation python script in code generators.
There was a problem hiding this comment.
@seberg Sure will look into that approach.
| @@ -4101,19 +4101,6 @@ class TestArgmax: | |||
| ([0, 1, 2, np.nan, 3], 3), | |||
| ([np.nan, 0, 1, 2, 3], 0), | |||
| ([np.nan, 0, np.nan, 2, 3], 0), | |||
| ([0, 1, 2, 3, complex(0, np.nan)], 4), | |||
There was a problem hiding this comment.
The removed tests should be put into test_deprecations.py I believe.
There was a problem hiding this comment.
Ah I see will look into that
|
@vrakesh Thank you for working on this PR! Do you think you could implement the suggestions above some time soon? |
|
@InessaPawson I will take a look at this soon (I will get to this by end of July) |
|
Actively working on this , will have it up as early as I can. |
|
Yeah, we had just talked about it 2 weeks or so ago, that maybe we can do this without worrying too much about having a short-hand replacement (i.e. lexsort being good enough). |
|
I believe the array API doesn't require complex ordering but doesn't restrict it either, but maybe @kgryte can confirm. |
|
@asmeurer Correct. As stated in the design document for complex numbers,
|
|
This depends on #16700, which we closed in 2023, so I'm closing this one as well. |
Depends on gh-16700
Follows up on the above PR. This PR deals with deprecation messages, and relevant tests that used complex type ordering comparisons