Conversation
579303b to
6602a20
Compare
6602a20 to
393934b
Compare
Remove unit tests for the view safety chekcs, which are to be reverted in the next commit.
4538d25 to
7ad5285
Compare
|
OK, I think this is good. Somewhat suprisingly the only issue that needs to be reopened is #2346, and I suspect even that will be fixed by #6053 without needing safety checks. So it looks like the safety checks weren't actually needed to solve any open issues - we just needed smarter logic to handle the case of object arrays in various places. Hmm... |
numpy/core/_internal.py
Outdated
There was a problem hiding this comment.
This looks a little funny. Why is it safe to return on first instance that the condition is satisfied?
There was a problem hiding this comment.
Just finding somthing that matches new type?
There was a problem hiding this comment.
The idea is to allow getfield as long as we are getting a field that already exists. Eg,
>>> ones(3, dtype='i4,O').setfield(5, 'O', 4)
Since get/setfield doesn't know the name of the field we have to search for the field based on the offset.
|
I like that so much code is removed ;) |
Because of slowdowns caused by the view safety checks introduced in numpy#5548 they are removed here for 1.10. The plan is to reintroduce a better version of the checks in 1.11.
7ad5285 to
796a5f8
Compare
|
removed the blank line |
|
@ahaldane Thanks for getting this done. And the removal of code always gives me a warm, fuzzy feeling ;) |
|
Thanks @charris, and agreed! Ultimately I think we will still want to allow view of arrays with objects at some point, but I'm hoping that will be easier under a new dtype system (in a year or two?), without needing these convoluted safety checks. |
|
Thanks @ahaldane ! I'm sorry this ended up being such a hassle for you. |
This PR disables the view safety checks that were causing problems in #6467, undoing some of the work from #5548. It goes back to the old behavior of disallowing any views/setfield involving object arrays.
This is better than reverting #5548, because it turns out there were other fixes in #5548 and subsequent PRs (eg #6208), plus in this PR, which fix most of the issues involving view safety without needing the safety checks. In fact, of the 5 issues fixed in #5548, only one needs to be re-opened due to this PR (#2599). (Edit: Actually #2346 needs to be re-opened too. Half of the examples there work now, but some are broken.)
I also request a day or two to sit on this before merging, please! (Ping me if I am delaying the release)