BUG: Ensure context path is taken in masked array array-wrap#27800
Merged
charris merged 1 commit intonumpy:mainfrom Nov 20, 2024
Merged
BUG: Ensure context path is taken in masked array array-wrap#27800charris merged 1 commit intonumpy:mainfrom
charris merged 1 commit intonumpy:mainfrom
Conversation
The unary comparison "domains" can return masked (the others can't so there is a dissonance here that doesn't really add up). This adds a force-cast to boolean, since the domain result only makes sense as boolean, but the `masked` scalar is float64... Now, why did this change in NumPy 2? It didn't... Both before and after a `TypeError` was raised (incorrectly) and the `context` path was then skipped completely (the ufunc machinery has a try/except). What changed is that I removed `__array_prepare__` because it had basically no use. Turns out, that in this error path (and only there) it was important because the "context" path was not taken, but `__array_prepare__` called `__array_finalize__` so it also applied the mask. For unary ufuncs, it thus neatly (but very round-about) fills the bad logic here. (For non-unary ufuncs, the "domain" functions don't return masked arrays so the path cannot happen) A revier with a keen eye may notice that the old code returned a masked array rather than a the masked constant here. Note, however, that this is special the incorrectly taken no-context path, because the other path doesn't have the "return scalar" logic (which is probably also a bug, but who is counting). Closes numpygh-25635
Member
|
Thanks Sebastian. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Below a story, that nobody needs to read, it is a clear fix. I am looking the integer cast difficulties, but it is still a bit difficult to unravel where to best do something about it.
(My preference would be to never store an incompatible
fill_valuebasically, which is likely easy enough, but I am not sure yet that fixes everything. Plus the easiest way to do that may be to ensure thatdefault_fill_value()returns something very different. That seems good, but maybe not for backporting.)The unary comparison "domains" can return masked (the others can't so there is a dissonance here that doesn't really add up).
This adds a force-cast to boolean, since the domain result only makes sense as boolean, but the
maskedscalar is float64...Now, why did this change in NumPy 2? It didn't... Both before and after a
TypeErrorwas raised (incorrectly) and thecontextpath was then skipped completely (the ufunc machinery has a try/except).What changed is that I removed
__array_prepare__because it had basically no use. Turns out, that in this error path (and only there) it was important because the "context" path was not taken, but__array_prepare__called__array_finalize__so it also applied the mask.For unary ufuncs, it thus neatly (but very round-about) fills the bad logic here.
(For non-unary ufuncs, the "domain" functions don't return masked arrays so the path cannot happen)
A revier with a keen eye may notice that the old code returned a masked array rather than the masked constant here. Note, however, that this is special the incorrectly taken no-context path, because the other path doesn't have the "return scalar" logic (which is probably also a bug, but who is counting).
Closes gh-25635