Repository navigation
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_getfieldandvoidtype_setfield. Instead they simply call the ndarraygetfieldandsetfield. 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_methodto 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_ToScalaralready does the byteswap for us. Therefore, any scalars that reachvoidtype_getfieldare 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,setfieldon a void scalar on a field ofObjecttype 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).