BUG: Fix deepcopy regression for empty arrays.#8704
Conversation
|
@seberg Could you take a look? |
numpy/core/src/multiarray/methods.c
Outdated
There was a problem hiding this comment.
this is not deallocated when the size is zero
numpy/core/src/multiarray/methods.c
Outdated
There was a problem hiding this comment.
Seems good to me, but the iterator clean up and the decrefs needs to be outside of the if block. Actually, another question (one for forwarding), can't the deepcopy inner loop fail, i which case we would have to chicken out?
There was a problem hiding this comment.
There is no error return in the inner loop. The _deepcopy_call function is void, although it calls itself recursively so there are returns for failures. It is a bit fishy there.
There was a problem hiding this comment.
Although I suppose we could check PyErr_Occurred(). It should be at least cleared if not handled...
There was a problem hiding this comment.
Well, if the inner loop does not shorten out on error, there is not much point. And if it would shorten out, then I would assume it had an error return.... Sounds like a bug, but probably for another day, since this is a regression anyway.
There was a problem hiding this comment.
Looked at it a bit more. If _deepcopy_call checked up top for PyErr_Occurred() and returned that would take care of the recursion. Whether or not to short circuit the loop after that is another question. Probably the cleanest fix would be to change the return to int and use that as an error indicator, which should be very quick to check.
There was a problem hiding this comment.
Yeah, I think the func needs an error indicator return, dunno if you actually need to check PyErr_Occured specifically or not (NULL return checks may do the trick, dunno). And should all work fine, assuming the array as NULLed out before. But I am good with pushing it to a different PR, that bug has probably be there since forever.
There was a problem hiding this comment.
Since about 2009... I opened an issue for it.
Deepcopy of empty arrays was failing because the nditer was constructed without the NPY_ITER_ZEROSIZE_OK flag. Closes numpy#8536.
ca0f744 to
d60ff8e
Compare
|
OK, should be fixed. |
|
Heh, I am confused if it was ever off ;), since there is this one indentation level more then I thought. Seems good to me, I think the check for iternext == NULL may even be unnecessary if you would check for a > 0 return, but that does not matter. |
|
Thanks Chuck. Just waiting for the tests to finish (or you can merge it if you beat me to it), the exception discussion belongs elsewhere in any case. |
|
@seberg Interesting. With size > 0 the |
|
I somewhat think the iternext function can only error on some rare things errors that can happen after the main setting up. IIRC I added that at some point, because of allowing to remove axis (the iterator can be "too large" before removal). If that is the case the size would return -1, so one of the two is good enough for error checking. But its safer to do the check in any case.... Anyway, would have to check the code to be sure, so its a bit of a silly trivia more then useful info.... |
Deepcopy of empty arrays was failing because the nditer was constructed
without the NPY_ITER_ZEROSIZE_OK flag.
Closes #8536.