Skip to content

BUG: Ensure context path is taken in masked array array-wrap#27800

Merged
charris merged 1 commit intonumpy:mainfrom
seberg:issue-25635
Nov 20, 2024
Merged

BUG: Ensure context path is taken in masked array array-wrap#27800
charris merged 1 commit intonumpy:mainfrom
seberg:issue-25635

Conversation

@seberg
Copy link
Member

@seberg seberg commented Nov 20, 2024

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_value basically, 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 that default_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 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 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

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
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Nov 20, 2024
@charris charris merged commit fc7cc1e into numpy:main Nov 20, 2024
@charris
Copy link
Member

charris commented Nov 20, 2024

Thanks Sebastian.

@seberg seberg deleted the issue-25635 branch November 20, 2024 20:22
@charris charris added this to the 2.1.4 release milestone Nov 24, 2024
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: sqrt(ma.masked) returns non-masked array

2 participants