BUG: fix incorrect output descriptor in fancy indexing #27715
Conversation
There was a problem hiding this comment.
Yes, this looks right extra_op is the result for indexing (things may be more interesting for assignments).
It's surprising why it's already used in the first branch, but not the second. If not a recent fix, maybe it's just a left-over of how the code grew, though.
I am surprised that the dtype mattered here, but probably it just is another way to trigger the full path (is to.e. a multi-dimensional advanced index would have done the same thing).
Shouldn't the test cover both short and large ones? But I agree, large are the interesting ones.
|
Yeah, this all dates back to when you originally wrote this code in 2022 according to the git history (unless that was a copy/paste). Probably just used to the idiom of assuming the input and output descriptor is identical in the indexing code.
Thanks for the suggestion, I expanded the test a bit and got another seg fault 🙃. Will push a supplementary fixup hopefully soon... |
|
Sorry, I think I have to go a bit deeper and think about it once more (should have read the comment closely). I think in this branch, we may have a set-up a cast from and we are setting up the first part only here, from However, EDIT: Sorry, I bargled up the first version, edited and added comment. |
|
On the "up-side": All of this should really only matter for |
|
It took me a while to reduce it, but this is the segfault I hit after expanding the tests: This dies inside |
|
Ugh, I think this runs into what you and I were talking about here #27057 (comment), which ultimately requires updating MapIter internals. |
|
Yeah, it had dawned to me also after the first too brief review. Should we try to figure that out together some time early next week? The issue isn't going away... |
|
Yeah if you'd be up for a pairing session in the morning my time and afternoon/evening your time I think that would be great. Monday is probably best for me but Wednesday works too. |
| npy_intp view_offset = NPY_MIN_INTP; | ||
| if (op[iop] != NULL && !(PyArray_SafeCast( | ||
| PyArray_DESCR(op[iop]), op_dtype[iop], &view_offset, | ||
| NPY_NO_CASTING, 1) && view_offset == 0)) { |
There was a problem hiding this comment.
I do wonder if we should have a clear way to spell it (up to changing the meaning of PyArray_EquivTypes to match this and changing the use where we want the meaning that allows string_dtype1 == string_dtype2 where needed).
There was a problem hiding this comment.
FWIW, after looking at it closer today, I realized that my worries /flip-back was in part due to getting the code paths exactly mixed up :).
The original fix looks very much right to me, although I won't swear that there are not more such problems around!
If you think it's all good, this seems fine to me (including for backporting) @ngoldbaum.
We may have a bunch more of such issues around EquivTypes though!
|
Assuming CI goes through OK I'll merge and open a followup to audit all the EquivTypes uses. If they're used to check casting safety they should be updated. |
|
I created #27751 to track the followups we identified before merging this. |
Fixes #27710. Fixes #27737.
Updates a casting check in nditer internals to a stricter check that considers view safety.