BUG: Use int for axes, not intp #8672
Conversation
numpy/core/src/multiarray/shape.c
Outdated
There was a problem hiding this comment.
We're kinda abusing PyArray_Dims here. This isn't a list of dims (intp), this is a list of axes (int).
There was a problem hiding this comment.
Why is it correct that the list of axes be int? Int may sufficient, but I doubt it is necessary.
There was a problem hiding this comment.
Looking at c-api.array.rst, ever occurence of axis is an int, not an intp. So from a consistency point of view, passing int would be better. Also, this bug wouldn't have existed if this used int in the first place like everything else.
int is not strictly necessary, but it is advisable. Of course, changing the public API here is a lot of work for little gain.
|
Probably should check (intp)axis == (int)axis ad raise if not, to avoid
uncontrolled downcast of user provided strange ints.
23.2.2017 1.50 "Eric Wieser" <[email protected]> kirjoitti:
… Rationale: typeof(PyArray_Dims.len) is int, so typeof(axis) should be the
same.
Fixes regression from #8584 <#8584>
------------------------------
You can view, comment on, or merge this pull request online at:
#8672
Commit Summary
- BUG: Use int for axes, not intp
File Changes
- *M* numpy/core/src/multiarray/nditer_pywrap.c
<https://github.com/numpy/numpy/pull/8672/files#diff-0> (2)
- *M* numpy/core/src/multiarray/shape.c
<https://github.com/numpy/numpy/pull/8672/files#diff-1> (8)
Patch Links:
- https://github.com/numpy/numpy/pull/8672.patch
- https://github.com/numpy/numpy/pull/8672.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8672>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACI5tYdACr5Y-gLN24_rpWynqGPujafks5rfNewgaJpZM4MJZQH>
.
|
There was a problem hiding this comment.
Hmm, changing the nditer code seems risky. Is there any reason it should not be npy_intp? A bit of extra precision is not harmful.
There was a problem hiding this comment.
Actually, this is already a bug, since not only are we assigning PyInt_AsLong to npy_intp (instead of PyArray_PyIntAsIntp, but we don't check for PyErr_Occured if the int doesn't fit in a long
There was a problem hiding this comment.
Hmm, this type of mistake crops up a few times - I'll push this to another PR
There was a problem hiding this comment.
And it seems that the overflow_check in PyArray_PyIntAsIntp_ErrMsg doesn't always run?
|
I'd rather not change established code if it isn't necessary. New code, OTOH... |
|
Not that it is wrong, but it increases risk, and NumPy at this point needs to be conservative. |
Rationale: typeof(PyArray_Dims.len) is int, so typeof(axis) should be the same.
eric-wieser
force-pushed
the
patch-8584
branch
from
86ed790 to
1dacacf
Compare
|
@pv: What error do you recommend I raise here? |
|
merging to get rid of the warning and crash. Adding a safeguard against rather odd input can be done later (OverflowError might be like a good exception). |
4 participants
Rationale: typeof(PyArray_Dims.len) is int, so typeof(axis) should be the same.
Fixes regression from #8584