BUG: Fix shape update in numpy.ctypeslib.as_array(pointer, shape) #6214
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
There was a problem hiding this comment.
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.
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.
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.
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.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
At this point I'd prefer a line break here, and below
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Note that we're no longer using TestCase - just use object here, and replace the self.assertEquals 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.