Repository navigation
BUG: Avoid optimization causing astropy regression - #31371
Conversation
Arguably, I think astropy is relying on things it shouldn't rely on, but let's not break them. I am considering adding a dict option here to enable the optimization on this path (i.e. allow setting the flag explicitly unless a subfield disables it). But if astropy wants this in quickly to unblock CI, this should be fine.
|
close/reopen |
|
Thanks Sebastian. The debug test seems unrelated. |
| raw_arr = np.zeros(10, dtype="i8") | ||
| arr1 = raw_arr.view(dtype) | ||
| arr2 = np.full(10, -1, dtype="i8").view(dtype) | ||
| # This should only fill in the i4 field of `dtype`: |
There was a problem hiding this comment.
Thanks for putting this test in. I'm not quite sure it is the same as what we do in uncertainty - will need to think! And in a way odd, because if the dtype were not identical, I think there is an argument to be made to disallow the optimization of also copying the extra 4 bytes, as there is no way to know what those entail.
But in the end, my use in astropy of a dtype as a way to effectively get different strides was perhaps not great... Unfortunately, it is pretty baked in, but I'll see if there is a way to make the dtype either fail is_dtype_struct_simple_unaligned_layout or make the copying safe for the cases that it passes.
There was a problem hiding this comment.
Interestingly, this particularly case would work if in the dtype definition one passed in offsets=[0]. For the astropy case, this also seems to work -- it effectively signals that one should do the alignment check and that one fails.
So, possibly that is another solution here? To run the alignment check even if offsets=NULL?
(Sorry, but too tired to think through now; maybe more tomorrow.)
There was a problem hiding this comment.
Not sure I am quite following, I don't think astropy uses offsets (but passing offsets can be common, I think even for cases where it isn't strictly needed maybe).
The test shows of course that if you use arr.view(dtype) then in a sense it depends on what the previous dtype was whether or not the view is actually safe w.r.t. to optimized assignment...
In that sense, the correct way might be to disable the flag on view (in theory unless we know no new "holes" are created).
Hmmmpf, tricky. For new arrays it is all straight forward, for views it seems a bit like it's maybe all tricky?
Arguably, I think astropy is relying on things it shouldn't rely on, but let's not break them.
I am considering adding a dict option here to enable the optimization on this path (i.e. allow setting the flag explicitly unless a subfield disables it).
But if astropy wants this in quickly to unblock CI, this should be fine.
Closes gh-31363