MAINT: more informative errors for 'axis' argument of ufuncs #5201
Conversation
| if (axis == -1 && error != NULL) { ; | ||
| /* | ||
| * Need to re-raise the exact same exception returned by | ||
| * PyArray_PyIntAsInt to not break the deprecation tests |
There was a problem hiding this comment.
I think it was these that were failing. By default assert_deprecated is looking for a DeprecationWarning, but it was getting a TypeError. We could fool around with the tests to make them pass and have a more direct error raising here, but it is probably not that bad of a thing that we re-raise what PyArray_PyIntAsInt is raising.
There was a problem hiding this comment.
Heh, I didn't bother with such replacement in a lot of places personally. It can be nice to show the deprecation error I guess, but not sure if it is worth juggling around in c-code to get there?
There was a problem hiding this comment.
Changing the test is easy, there is an argument in that function which you can tell that the error is a TypeError and not a Deprecation one.
There was a problem hiding this comment.
Looking at this again, the comment seems out of place. But I now more strongly think that we cannot unconditionally raise a TypeError, especially after considering what happens with integer overflows. On my 32 bit numpy I get:
>>> np.random.rand(10, 20, 30).sum(axis=(2**32,))
Traceback (most recent call last):
...
OverflowError: 'axis' tuple entries must be integers
>>> np.random.rand(10, 20, 30).sum(axis=2**32)
Traceback (most recent call last):
...
OverflowError: 'axis' must be None, an integer or a tuple of integers
Which are anything but informative! 64 bit systems with 4 byte ints could also show a ValueError, if the value overflows an int but not a npy_intp.
The proper thing to do would be to call PyArray_IntAsInt_ErrMsg directly, not through PyArray_IntAsInt, but since it is not part of the API I am not sure of what is best here: checking the error type and letting ValueError and OverflowError pass unchanged, and modify the message for other errors feels hacky and fragile, but seems the only other way to correctness.
A more elaborate possibility is to add PyArray_ConvertMultiAxis to the API, and adapt it slightly (return naxes also) so that it can be used here directly.
Thoughts?
There was a problem hiding this comment.
integer overflows
Is this a serious concern? the largest possible axis index is 31... (not 2**31, just 31).
A more elaborate possibility is to add PyArray_ConvertMultiAxis to the API, and adapt it slightly (return naxes also) so that it can be used here directly.
I'll point out that this is Yet Another case where we are considering exposing boring utility functions with funky calling conventions in the API solely so we can use them internally in both multiarray and umath.
| if (axis == -1 && error != NULL) { | ||
| /* | ||
| * Need to re-raise the exact same exception returned by | ||
| * PyArray_PyIntAsInt to not break the deprecation tests |
There was a problem hiding this comment.
Which deprecation tests? Are those deprecation tests still valid? Some deprecations have reached their due date.
There was a problem hiding this comment.
I ask, because I'd rather avoid temporary complications in the code like this that may never get resimplified.
|
This seems simple enough with some caveats. |
|
☔ The latest upstream changes (presumably #8584) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@jaimefrio: There's now (#8584) a Right now, the implementation does not add any context to the exception, such as the fact that it might be an item within a larger tuple, as you do here. If you want to pursue this, I'd be for adding another parameter for any extra message information. |
|
Closing since we unified axis error messages differently. Please refactor into a new PR if I missed something. |
This follows from the discussion in #5196, and unifies the errors returned by
PyArray_ConvertMultiAxisinconversion_utils.c, with the similar code inufunc_object.cto handle theaxisparameter to ufunc reduction methods.