-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: Fix shape update in numpy.ctypeslib.as_array(pointer, shape) #6214
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.
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
Conversation
Needs a test. This doesn't solve the problem anyway, because there's another |
In Will add a test for it. |
I added tests for |
numpy/ctypeslib.py
Outdated
except AttributeError: pass | ||
except TypeError: return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this catching?
I might be tempted to write this as:
try:
interface = pointer_obj.__array_interface__
except AttributeError:
pass
else:
interface['shape'] = shape # catch TypeError here, if you're sure doing so is needed
return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AttributeError
identifies the absent of __array_interface__
. The TypeError
states that __array_interface__
is not a dictionary and __array_interface__['shape'] = shape
failed. Returning from this will delegate the error handling to np.array
.
Personally I think, that having another error handling block to handle the second property, is a redundancy that should be skipped. But maybe it makes sense to document the meaning of each exception.
try: pointer_obj.__array_interface__['shape'] = shape
# __array_interface__ needs to be setup
except AttributeError: pass
# np.array should handle non-dict __array_interface__
except TypeError: return
# __array_interface__ is setup and shape was updated
else: return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is __array_interface__
allowed to not be a dictionary? Seems to me that we should not silence that error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having separate error handlers is not a redundancy - it makes it explicit about which things are allowed to raise which errors. If pointer_obj.__array_interface__
raises a TypeError
, then presumably something is very wrong, and we should not silence it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The __array_interface__
variable could be something else. It might be an issue with the user-implementation and thus possible. I just don't want to make an assumption of which is right or wrong. In this method we only prepare __array_interface__
as good as possible. Just after this np.array
is called to wrap the pointer and this method will raise a more meaningful error if __array_interface__
is something that cannot be handled. Additionally __array_interface__
was not set by prepare_pointer
in this case, so assuming that setting the shape should work, might be wrong.
If you'd prefer to have the separation of the two possible errors, I can implement it that way. I just don't like it in the sense of readability and verbosity.
Interesting test failure there... Looks like somewhere we're silencing |
Yes, it seems like What should I do about the test decorators |
numpy/tests/test_ctypeslib.py
Outdated
@@ -3,9 +3,10 @@ | |||
import sys | |||
|
|||
import numpy as np | |||
from numpy.ctypeslib import ndpointer, load_library | |||
from ctypes import c_int, cast, POINTER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is OK, based on #8898 requiring ctypes
to not be imported at the module level. Do any other tests import ctypes at this level?
numpy/ctypeslib.py
Outdated
@@ -401,9 +401,13 @@ def prep_pointer(pointer_obj, shape): | |||
attach an __array_interface__ property to it if it does not | |||
yet have one. | |||
""" | |||
try: pointer_obj.__array_interface__ | |||
try: inter = pointer_obj.__array_interface__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I'd prefer a line break here, and below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the look of prep_pointer
at all, before or after this patch. Perhaps it should change to returning a copy of the input point, which it seems you can do with type(pointer_obj)(pointer_obj.contents)
numpy/tests/test_ctypeslib.py
Outdated
@@ -113,6 +113,32 @@ def test_cache(self): | |||
a2 = ndpointer(dtype=np.float64) | |||
self.assertEqual(a1, a2) | |||
|
|||
class TestAsArray(TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we're no longer using TestCase
- just use object
here, and replace the self.assertEqual
s below with assert_equal
Missing newline above too, while you're here
Sorry for dropping the ball here! Found this via https://pav.iki.fi/numpy-needs-work/ |
This needs a rebase / merge |
Worrying error:
|
Everything behaves a lot better if we let the array constructor handle it, which will use the ctypes PEP3118 support. Bugs this fixes: * Stale state being attached to pointer objects (fixes numpygh-2671, closes numpygh-6214) * Weird failure modes on structured arrays (fixes-10978) * A regression in numpygh-10882 (fixes numpygh-10968)
Everything behaves a lot better if we let the array constructor handle it, which will use the ctypes PEP3118 support. Bugs this fixes: * Stale state being attached to pointer objects (fixes numpygh-2671, closes numpygh-6214) * Weird failure modes on structured arrays (fixes-10978) * A regression in numpygh-10882 (fixes numpygh-10968)
Fixes #2671
It updates the shape if
__array_interface__
is a dict-like object.Otherwise it behaves the same as before.