MAINT: Use the same exception for all bad axis requests #8584

eric-wieser · 2017-02-08T18:19:36Z

Lots of functions accept an axis argument that can be negative, implying wraparound. The same few lines of code are repeated all over the place to implement this.

When the axis is just plain invalid, an error is raised. However:

Some things raise IndexError, others raise ValueError. This PR somehwat arbitrarily assumes that IndexError is the right type of exception to raise.
Some messages are more useful than others
Some functions don't check for axis < -ndim

It seems it would be better to put all this in one place.

This was previously filed as an issue at #8583. Function name and location inspired by check_and_adjust_index. check_and_adjust_index(...) < 0 is used to match check_and_adjust_axis(...) < 0.

This was previously discussed on the mailing list by @charris and @seberg

mhvk · 2017-02-08T18:58:49Z

numpy/core/src/multiarray/common.h

+ * 0 <= *axis < ndim, and returns 0.
+ */
+static NPY_INLINE int
+check_and_adjust_axis(int *axis, int ndim)


adjust seems to imply something always happens; how about check_and_normalize_axis. Or just normalize_axis?

mhvk · 2017-02-08T19:01:43Z

numpy/core/src/multiarray/conversion_utils.c

-                PyErr_Format(PyExc_ValueError,
-                        "'axis' entry %d is out of bounds [-%d, %d)",
-                        axis_orig, ndim, ndim);
+            if (check_and_adjust_axis(&axis, ndim) < 0) {


Here and below: check for status using != 0? Or just omit the comparison altogether (as above in error_converting(axis)

mhvk · 2017-02-08T19:02:21Z

numpy/core/src/multiarray/conversion_utils.c

-                    axis_orig, ndim, ndim);
-            return NPY_FAIL;
+        if (check_and_adjust_axis(&axis, ndim) < 0) {
+            return NULL;


Shouldn't one still return NPY_FAIL here?

mhvk · 2017-02-08T19:03:14Z

numpy/core/src/multiarray/item_selection.c

-        PyErr_Format(PyExc_ValueError, "axis(=%d) out of bounds", axis_orig);
-        return -1;
+    if (check_and_adjust_axis(&axis, n) < 0) {
+        return NULL;


Same here: I think the return argument shouldn't change, i.e., return -1 here?

mhvk · 2017-02-08T19:03:28Z

numpy/core/src/multiarray/item_selection.c

-        PyErr_Format(PyExc_ValueError, "axis(=%d) out of bounds", axis_orig);
-        return -1;
+    if (check_and_adjust_axis(&axis, n) < 0) {
+        return NULL;


mhvk · 2017-02-08T19:04:47Z

numpy/core/src/multiarray/item_selection.c

-                "axis(=%d) out of bounds", axis);
-        goto fail;
+    if (check_and_adjust_axis(&axis, nd) < 0) {
+        return NULL;


Definitely should be goto fail here, since several DECREFs need to be done.

mhvk · 2017-02-08T19:06:40Z

numpy/core/src/multiarray/multiarraymodule.c

@@ -4109,6 +4103,24 @@ array_may_share_memory(PyObject *NPY_UNUSED(ignored), PyObject *args, PyObject *
    return array_shares_memory_impl(args, kwds, NPY_MAY_SHARE_BOUNDS, 0);
 }

+static PyObject *


I presume this is for follow up in python code? If so, I think it should be part of a follow-up PR, not be done here (if only to give a chance to discuss whether it is best to go through C for something like this).

I've added the python stuff to this PR now...

mhvk · 2017-02-08T19:21:46Z

This makes sense to me, but note that I think you'll have to ensure the code keeps behaving the same in case of failure. As you will have seen, I suggest postponing the method to a possible python follow-up PR.

There are also a few more places where the code could be used:

mhvk · 2017-02-08T19:23:14Z

Actually, looking more closely at anything that includes axis.*<.*0, I find there is already a method which would seem to do what you want... check_and_adjust_index -- https://github.com/numpy/numpy/blob/master/numpy/core/src/multiarray/common.h#L110

mhvk · 2017-02-08T19:25:27Z

p.s. Well, the function is very similar but doesn't exactly do the same thing, since that one checks an index rather than an axis, but with the same logic. I'm not sure I understand the threading stuff...

mhvk · 2017-02-08T19:26:19Z

p.p.s. And I really should learn to read commit messages -- you of course refer to check_and_adjust_index already...

eric-wieser · 2017-02-08T22:19:36Z

Wow, that was a clumsy bunch of mistakes. Thanks for catching me out!

you of course refer to check_and_adjust_index already...

Yep - I've addressed your concerns in an edit to the original post.

There are also a few more places where the code could be used:

Great, thanks for spotting those - I'll add a fixup commit for those shortly

you'll have to ensure the code keeps behaving the same in case of failure.

Meaning what? Produce the same error message?

mhvk · 2017-02-08T23:02:35Z

you'll have to ensure the code keeps behaving the same in case of failure.

Meaning what? Produce the same error message?

No, just that what the code returns (error code or otherwise) remains the same (see in-line comments)

mhvk · 2017-02-09T00:20:53Z

I like how much the code cleans up using this! A few things I'm uncertain about:

For the python side, a pure python implementation would have the advantage that it is easier to see what it does (this is partially why I thought it might be good to do it in a separate PR);
For the python side, I'd prefer a name that makes more obvious what it does, e.g., normalize_axis);
Am not quite sure what the appropriate error is; people may count on getting ValueError, though I would agree that IndexError is more logical.

eric-wieser · 2017-02-09T00:25:38Z

@mhvk

I don't like the idea of implementing the same code in both C and python, leaving the only alternative as having the C code call python - but that would be a performance hit
I've deliberately not exposed it publically at np.resolve_axis because I was not sure of the name - I'd welcome some more input - I don't have a good argument for one or the other
I'd be in favor of making a special AxisError type, because that would make the tests more explicit. Of course, the question then becomes "should this subclass ValueError, IndexError, or just Exception"...

charris · 2017-02-09T01:04:36Z

Grrr, github is now notifying me of commits without referencing a PR. It's an "improvement" too far.

eric-wieser · 2017-02-09T01:07:40Z

@charris : that's my fault for doing some url mangling in order to comment on a commit and not a diff line

mhvk · 2017-02-09T01:11:16Z

Hmm, possibly one needs class AxisError(IndexError, ValueError)...

mhvk · 2017-02-09T01:15:37Z

More seriously: which MaskedArray test is actually broken by the change from ValueError to IndexError -- I tried looking at the travis results, but only noticed 2 failures, which got me confused (maybe I just looked badly).

eric-wieser · 2017-02-09T01:19:54Z

@mhvk: Look in the test results before the commit that claims to fix them ;)

eric-wieser · 2017-02-09T01:28:55Z

numpy/core/numeric.py

@@ -1527,15 +1527,12 @@ def rollaxis(a, axis, start=0):

    """
    n = a.ndim
-    if axis < 0:
-        axis += n
+    axis = resolve_axis(axis, n)
    if start < 0:
        start += n


This has always struck me as a bug - IMO, rollaxis(np.zeros(3,4,5), 0, -1).shape == (4, 5, 3), ie -1 should mean "move this axis to the end".

This would be consistent with the way that np.stack(..., axis=-1) means insert an axis at the end. The fact that this is the only place that can't use resolve_axis sort of backs up the fact that this is non-intuitive and breaks the pattern.

I would like this to be written start = resolve_axis(axis, n+1), but that would be a breaking change.

Yes, I have been caught by that too, realising only slowly that I had to put array.ndim if I wanted something moved to the last axis. I think this is partially why we now have moveaxis -- in any case, I think it is fine to leave this one untouched!

mhvk · 2017-02-09T01:31:45Z

I did... E.g., https://travis-ci.org/numpy/numpy/jobs/199817411 (2 errors only). But now I look at the appveyor results, I see that there are also 3 errors in test_recfunctions. Most bizarre. Maybe travis deletes jobs sooner? Or the github link is incorrect?

eric-wieser · 2017-02-09T01:34:04Z

This appveyor page shows the bug

travis now fails due to warnings.

mhvk · 2017-02-09T03:25:55Z

@eric-wieser - with #8590, you should not need to fix ma/core.py any more.

eric-wieser · 2017-02-09T10:42:44Z

you should not need to fix ma/core.py any more.

I still need to swap ValueError for IndexError for the tests to pass. But yes, the change will be overwritten when you merge that PR

shoyer · 2017-02-09T17:38:27Z

I don't think IndexError is quite right here, because that exception is quite specifically for subscripting a sequence. I would lean towards class AxisError(ValueError), or maybe class AxisError(IndexError, ValueError) if we're concerned about backwards compatibility.

eric-wieser · 2017-02-09T17:55:29Z

because that exception is quite specifically for subscripting a sequence

Are we not essentially subscripting arr.shape[axis] here though? I suspect that was the argument being used in the past for IndexError

What would be an appropriate name/prefix for AxisError in the C code, and which file would it be best defined in?

mhvk · 2017-02-09T17:58:54Z

I think I'd also pick IndexError over ValueError as we're indexing the array's dimensions. But I worry about breaking code in either case... That said, the fact that so little broke in numpy by doing this is a good sign (the MaskedArray one I'd chalk up to pure coincidence).

eric-wieser · 2017-02-20T22:04:23Z

Ok, renamed to np.core.multiarray.normalize_axis_index, and a docstring added.

seberg · 2017-02-21T12:53:03Z

I did not follow this much, but I have a believe that someone will find any weird function and think its fine to use it. So, unless we are fine with people possibly starting to use it, adding an underscore to underscore ;) that its considered private just seems like a save bet.

eric-wieser · 2017-02-21T13:13:25Z

@seberg: I'd argue that this is something we actually want people to use - for anyone writing their own function taking an axis argument that can't just forward it, this is a simple way to make it behave as people expect.

seberg · 2017-02-21T14:59:10Z

Ok, sounds fine to me, it was more of an "if we are not sure its stable API" thing, to be honest.

mhvk · 2017-02-21T16:36:23Z

I think I agree with @eric-wieser - it is good if this gets used (but would not quite yet put it at the top level)

mhvk · 2017-02-21T20:58:05Z

Thanks for adding the docstring; I think all is set now, so will merge.

eric-wieser · 2017-02-21T21:05:56Z

Should this have had a release note, or will that be handled during the release stage?

mhvk · 2017-02-21T21:17:28Z

Hmm, yes, probably big enough to have a release note... Now best to do separately?

charris · 2017-02-22T20:24:04Z

Yes, should have a release note as it is a behavior change. The type warning for the check_and_adjust_axis call also needs a fix that would be good to get in before it is forgotten.

charris · 2017-02-22T21:48:40Z

@mhvk It's in, so will need a separate PR for the release note.

pv · 2017-02-23T00:06:02Z

This appears to cause a crash in scipy, gh-8671

pv · 2017-02-23T00:07:06Z

numpy/core/src/multiarray/shape.c

-            if (axis < 0 || axis >= PyArray_NDIM(ap)) {
-                PyErr_SetString(PyExc_ValueError,
-                                "invalid axis for this array");
+            if (check_and_adjust_axis(&axis, PyArray_NDIM(ap)) < 0) {


axis is npy_intp, not int.

Maybe useful to double check this everywhere.

Which do you think I should accept in check_and_adjust_axis? Sometimes axis can be int, sometimes it is intp. Obviously I can introduce some locals for conversion, but I still need to choose which is right

charris · 2017-02-23T00:28:37Z

@eric-wieser Time to get going on that fix.

eric-wieser · 2017-02-23T00:34:42Z

@charris: Will do that ~~tomorrow~~ now #8672

[ci skip]

DOC: Add missing release note for #8584

eric-wieser force-pushed the resolve_axis branch 2 times, most recently from 730f87c to 7ec4d8f Compare February 8, 2017 18:48

mhvk reviewed Feb 8, 2017

View reviewed changes

eric-wieser force-pushed the resolve_axis branch from 7ec4d8f to 29c22a7 Compare February 8, 2017 22:09

eric-wieser changed the title ~~MAINT: Use the same error message for most C code handling negative axes~~ MAINT: Use the same exception for all bad axis requests Feb 8, 2017

eric-wieser force-pushed the resolve_axis branch 2 times, most recently from a9c76e6 to 353107a Compare February 9, 2017 00:12

eric-wieser commented Feb 9, 2017

View reviewed changes

mhvk mentioned this pull request Feb 9, 2017

Questionable behaviour in MaskedArray.__eq__ on flexible arrays #8589

Closed

eric-wieser force-pushed the resolve_axis branch from 004f72c to de3a75f Compare February 9, 2017 10:40

mhvk added this to the 1.13.0 release milestone Feb 21, 2017

mhvk merged commit 2aabeaf into numpy:master Feb 21, 2017

pv reviewed Feb 23, 2017

View reviewed changes

eric-wieser mentioned this pull request Feb 23, 2017

BUG: Use int for axes, not intp #8672

Merged

eric-wieser mentioned this pull request Mar 22, 2017

BUG: fix the error msg of empty hstack input #8801

Merged

eric-wieser added a commit to eric-wieser/numpy that referenced this pull request Mar 25, 2017

DOC: Add missing release note for numpy#8584

d70c7ae

[ci skip]

eric-wieser mentioned this pull request Mar 25, 2017

DOC: Add missing release note for #8584 #8828

Merged

charris added a commit that referenced this pull request Mar 25, 2017

Merge pull request #8828 from eric-wieser/8584-release-notes

c852077

DOC: Add missing release note for #8584

eric-wieser mentioned this pull request Mar 26, 2017

MAINT: Use AxisError in more places #8843

Merged

eric-wieser mentioned this pull request May 11, 2017

Behaviour change in 1.13 for expand_dims #9100

Closed

ahaldane mentioned this pull request Oct 6, 2018

Inconsistent concatenate behaviour #5759

Closed

eric-wieser mentioned this pull request Jul 19, 2021

ENH: Add the axis and ndim attributes to np.AxisError #19459

Merged

MAINT: Use the same exception for all bad axis requests #8584

MAINT: Use the same exception for all bad axis requests #8584

Conversation

eric-wieser commented Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented Feb 8, 2017

mhvk commented Feb 8, 2017

mhvk commented Feb 8, 2017

mhvk commented Feb 8, 2017

eric-wieser commented Feb 8, 2017 • edited Loading

mhvk commented Feb 8, 2017

mhvk commented Feb 9, 2017

eric-wieser commented Feb 9, 2017

charris commented Feb 9, 2017

eric-wieser commented Feb 9, 2017

mhvk commented Feb 9, 2017

mhvk commented Feb 9, 2017

eric-wieser commented Feb 9, 2017

eric-wieser Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented Feb 9, 2017

eric-wieser commented Feb 9, 2017 • edited Loading

mhvk commented Feb 9, 2017

eric-wieser commented Feb 9, 2017

shoyer commented Feb 9, 2017

eric-wieser commented Feb 9, 2017 • edited Loading

mhvk commented Feb 9, 2017

eric-wieser commented Feb 20, 2017

seberg commented Feb 21, 2017

eric-wieser commented Feb 21, 2017

seberg commented Feb 21, 2017

mhvk commented Feb 21, 2017

mhvk commented Feb 21, 2017

eric-wieser commented Feb 21, 2017

mhvk commented Feb 21, 2017

charris commented Feb 22, 2017

charris commented Feb 22, 2017

pv commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

charris commented Feb 23, 2017

eric-wieser commented Feb 23, 2017 • edited Loading

eric-wieser commented Feb 8, 2017 •

edited

Loading

eric-wieser commented Feb 8, 2017 •

edited

Loading

eric-wieser Feb 9, 2017 •

edited

Loading

eric-wieser commented Feb 9, 2017 •

edited

Loading

eric-wieser commented Feb 9, 2017 •

edited

Loading

eric-wieser Feb 23, 2017 •

edited

Loading

eric-wieser commented Feb 23, 2017 •

edited

Loading