BUG: Relax dtype identity check in reductions#20729
Conversation
seberg
force-pushed
the
relax-reduce-dtype-identity-check
branch
from
88f728a to
7e1b9bc
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This looks good, but also definitely needs a test case so that it doesn't recur!
numpy/core/src/umath/ufunc_object.c
Outdated
| * be identical. The second argument can be different for reductions, | ||
| * but is checked to be identical for accumulate and reduceat. | ||
| * Ideally, the type-resolver ensures that all are identical, but we do | ||
| * not enforce this here strictly. |
There was a problem hiding this comment.
I would add "strictly, since on windows this seems not to be guaranteed; see gh-20699"
|
I ran into an caching(?) issue during testing :/ will have to dig into that as well. |
In some cases, e.g. ensure-native-byte-order will return not the default, but a copy of the descriptor. This (and maybe metadata) makes it somewhat annoying to ensure exact identity between descriptors for reduce "operands" as returned by the resolve-descirptors method of the ArrayMethod. To avoid this problem, we check for no-casting (which implies viewable with `offset == 0`) rather than strict identity. Unfortunately, this means that descriptor resolution must be slightly more careful, but in general this should all be perfectly well defined. Closes numpygh-20699
seberg
force-pushed
the
relax-reduce-dtype-identity-check
branch
from
7e1b9bc to
975a946
Compare
|
Well, that other issue is just that: another issue. I added the test, arguably it is a bit of an indirect test, but at least it tests the most relevant and interesting error-path. |
|
Thanks @seberg |
|
After merging this to main, the debug test runs started segfaulting. @seberg any ideas? |
|
Hummm, how come CI didn't notice that ahead of time? It is an abort, so maybe likely be an assert gone wrong. |
seberg
added
09 - Backport-Candidate
charris
added
component: numpy._core
and removed
09 - Backport-Candidate
|
Just to note that this fixes another astropy issue, astropy/astropy#12706. This one is in considerably more common code - @charris: would it be possible to do a relatively quick 1.22.1?? |
|
I think that would be fine, but there are one or two other issues and I was too distracted to make progress. I was hoping I can fix both this week so we would be ready for a 1.22.1 at the end of the week or next. But if it is much the same for Chuck, we can also just aim for a 1.22.2 soon after. IIRC the other issue was not found by anyone (but could be important!), and I don't really think we need to scramble about breaking numba (they just got 1.21 support recently). |
|
OK, sounds good! It definitely doesn't hang on a week. |
4 participants
In some cases, e.g. ensure-native-byte-order will return not the
default, but a copy of the descriptor.
This (and maybe metadata) makes it somewhat annoying to ensure
exact identity between descriptors for reduce "operands" as
returned by the resolve-descirptors method of the ArrayMethod.
To avoid this problem, we check for no-casting (which implies
viewable with
offset == 0) rather than strict identity.Unfortunately, this means that descriptor resolution must be slightly
more careful, but in general this should all be perfectly well
defined.
Closes gh-20699