-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: simplify void scalar getfield/setfield #5642
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
Be good to post the description to the list so more folks see it. |
I'll post it, probably later this week |
f0a0d89
to
553bcd9
Compare
I updated it so it no longer breaks backwards compatibility in scipy. I belive all failed tests in both scipy and numpy would now be fixed by #5548. Why the change? I realized that the new behavior was the wrong one: Setitem and setfield on structured object arrays should behave identically:
Both assignments now produce
Now that there is not such a serious problem, is it still worth posting to the mailing list? One thing that seems a little hacky in the new code is the way I assign to the 0-d array (which may actually be a higher dim array in the case of subarrays). I effectively do
Is there a better way of assigning to 'everything' that doesn't involve creating the empty tuple passign it to |
553bcd9
to
60ef8a7
Compare
This commit modifiesvoidtype_get/setfield to that they call the ndarray get/setfield. This solves bugs related to void-scalar assignment. This fixes issues numpy#3126, numpy#3561.
60ef8a7
to
6c7e75d
Compare
BTW, you need separate |
@ahaldane Needs rebase. |
I made a fresh re-start for this PR in #5947 |
This PR removes most of the logic in
voidtype_getfield
andvoidtype_setfield
. Instead they simply call the ndarraygetfield
andsetfield
. This solves bugs related to void-scalar assignmentThis fixes #3126, #3561. An alternate PR for solving these issues is #4556.
However, this PR introduces a backwards incompatible change, which causes some current scipy tests to fail: Assignment to a void scalar's object array fields now unpacks any arrays on the lhs of the assignment, unlike before (details below).Edit: No longer a problemIn addition, this PR would eliminate potential segfaults involving object arrays if applied in combination with my other PR #5548. This PR currently does not pass numpy tests for this reason, as it exposes a potential bugs that were silent before. Specifically, in this PR, some tests now raise errors of the form "Cannot change data-type for object array" because they attempt to directly modify memory in object arrays. This was previously silently allowed, but now raises an error (which I think is better). My other PR would elimitate the errors. Probably my other PR should be considered before this one is.
More detailed justification for the changes:
Removed byteswapping code
I removed code that does byteswapping in both getfield and setfield. After looking at it for a while, I am convinced this code is unnecessary (though this definitely needs to be checked by others). The reason is that in both cases we use
gentype_generic_method
to convert the void scalar to a 0-d ndarray, do the get/setfield, and then convert the returned value to a scalar usingPyArray_ToScalar
(inPyArray_Return
). However, note thatPyArray_ToScalar
already does the byteswap for us. Therefore, any scalars that reachvoidtype_getfield
are already in NBO and there is no need to swap again.Change to object array assignment
This PR causes setfield called to behave differently in the specific case where the user tries to set a field of Object dtype, on a void scalar, to a value which is an ndarray. This causes some scipy tests to fail, involving loading matlab save files. The following example demonstrates what is going on:Setup:Old Behavior:New Behavior:In the old behavior,setfield
on a void scalar on a field ofObject
type was a special case in numpy code - if the value being assigned was an ndarray it would not be broadcast, unlike all other types of assignment where it would be, and unlike for non-object dtypes. I think there is a case to be made that the new behavior is more consistent - it does not have the (undocumented) 'special case'. On the other hand, this seems to have been the way scipy uses to set an object field with an ndarray as the value without broadcasting the ndarray - scipy code would have to change. It is a bit tricker to do that now - but still possible, eg withst2[0,0] = (None, v)
.