8000 MAINT: more informative errors for 'axis' argument of ufuncs by jaimefrio · Pull Request #5201 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: more informative errors for 'axis' argument of ufuncs #5201

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

MAINT: more informative errors for 'axis' argument of ufuncs #5201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions numpy/core/src/multiarray/conversion_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ PyArray_ConvertMultiAxis(PyObject *axis_in, int ndim, npy_bool *out_axis_flags)
for (i = 0; i < naxes; ++i) {
PyObject *tmp = PyTuple_GET_ITEM(axis_in, i);
int axis = PyArray_PyIntAsInt_ErrMsg(tmp,
"integers are required for the axis tuple elements");
"'axis' tuple entries must be integers");
int axis_orig = axis;
if (error_converting(axis)) {
return NPY_FAIL;
Expand Down Expand Up @@ -289,7 +289,7 @@ PyArray_ConvertMultiAxis(PyObject *axis_in, int ndim, npy_bool *out_axis_flags)
memset(out_axis_flags, 0, ndim);

axis = PyArray_PyIntAsInt_ErrMsg(axis_in,
"an integer is required for the axis");
"'axis' must be None, an integer or a tuple of integers");
axis_orig = axis;

if (error_converting(axis)) {
Expand Down
32 changes: 26 additions & 6 deletions numpy/core/src/umath/ufunc_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3745,7 +3745,16 @@ PyUFunc_GenericReduction(PyUFuncObject *ufunc, PyObject *args,
for (i = 0; i < naxes; ++i) {
PyObject *tmp = PyTuple_GET_ITEM(axes_in, i);
int axis = PyArray_PyIntAsInt(tmp);
if (axis == -1 && PyErr_Occurred()) {
int axis_orig = axis;
PyObject *error = PyErr_Occurred();
if (axis == -1 && error != NULL) { ;
/*
* Need to re-raise the exact same exception returned by
* PyArray_PyIntAsInt to not break the deprecation tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what tests are these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
PyErr_Clear();
PyErr_SetString(error,
"'axis' tuple entries must be integers");
Py_XDECREF(otype);
Py_DECREF(mp);
return NULL;
Expand All @@ -3754,8 +3763,9 @@ PyUFunc_GenericReduction(PyUFuncObject *ufunc, PyObject *args,
axis += ndim;
}
if (axis < 0 || axis >= ndim) {
PyErr_SetString(PyExc_ValueError,
"'axis' entry is out of bounds");
PyErr_Format(PyExc_ValueError,
"'axis' entry %d is out of bounds [-%d, %d)",
axis_orig, ndim, ndim);
Py_XDECREF(otype);
Py_DECREF(mp);
return NULL;
Expand All @@ -3766,8 +3776,17 @@ PyUFunc_GenericReduction(PyUFuncObject *ufunc, PyObject *args,
/* Try to interpret axis as an integer */
else {
int axis = PyArray_PyIntAsInt(axes_in);
int axis_orig = axis;
PyObject *error = PyErr_Occurred();
/* TODO: PyNumber_Index would be good to use here */
if (axis == -1 && PyErr_Occurred()) {
if (axis == -1 && error != NULL) {
/*
* Need to re-raise the exact same exception returned by
* PyArray_PyIntAsInt to not break the deprecation tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which deprecation tests? Are those deprecation tests still valid? Some deprecations have reached their due date.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ask, because I'd rather avoid temporary complications in the code like this that may never get resimp ED48 lified.

*/
PyErr_Clear();
PyErr_SetString(error,
"'axis' must be None, an integer or a tuple of integers");
Py_XDECREF(otype);
Py_DECREF(mp);
return NULL;
Expand All @@ -3780,8 +3799,9 @@ PyUFunc_GenericReduction(PyUFuncObject *ufunc, PyObject *args,
axis = 0;
}
else if (axis < 0 || axis >= ndim) {
PyErr_SetString(PyExc_ValueError,
"'axis' entry is out of bounds");
PyErr_Format(PyExc_ValueError,
"'axis' entry %d is out of bounds [-%d, %d)",
axis_orig, ndim, ndim);
Py_XDECREF(otype);
Py_DECREF(mp);
return NULL;
Expand Down
9 changes: 9 additions & 0 deletions numpy/core/tests/test_ufunc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,6 +1073,9 @@ def test_reduce_arguments(self):
assert_equal(f(d), r)
# a, axis=0, dtype=None, out=None, keepdims=False
assert_equal(f(d, axis=0), r)
assert_equal(f(d, axis=None), 10)
assert_equal(f(d, axis=(0, 1)), 10)
assert_equal(f(d, axis=(-1, -2)), 10)
assert_equal(f(d, 0), r)
assert_equal(f(d, 0, dtype=None), r)
assert_equal(f(d, 0, dtype='i'), r)
Expand All @@ -1098,6 +1101,12 @@ def test_reduce_arguments(self):
assert_raises(TypeError, f, d, axis="invalid")
assert_raises(TypeError, f, d, axis="invalid", dtype=None,
keepdims=True)
assert_raises(TypeError, f, d, axis=[-1, -2])
assert_raises(TypeError, f, d, axis=(-1, 'invalid'))
assert_raises(ValueError, f, d, axis=(0, -3))
assert_raises(ValueError, f, d, axis=(0, 2))


# invalid dtype
assert_raises(TypeError, f, d, 0, "invalid")
assert_raises(TypeError, f, d, dtype="invalid")
Expand Down
0