BUG: Make mask_invalid consistent with mask_where if copy is set to False
#22046
Conversation
…e. Add test for type erroring.
mask_invalid consistent with mask_where if copy is set to Falsemask_invalid consistent with mask_where if copy is set to False
numpy/ma/core.py
Outdated
| try: | ||
| return masked_where(~(np.isfinite(getdata(a))), a, copy=copy) | ||
| except TypeError: | ||
| raise |
There was a problem hiding this comment.
Was there a particular reason for this try/except, or is it a debug remnant?
| try: | |
| return masked_where(~(np.isfinite(getdata(a))), a, copy=copy) | |
| except TypeError: | |
| raise | |
| return masked_where(~(np.isfinite(getdata(a))), a, copy=copy) |
There was a problem hiding this comment.
Indeed, I can't recall why I put the try/except. Suggestion applied. Thanks!
| with pytest.raises(TypeError, | ||
| match="not supported for the input types"): | ||
| np.ma.masked_invalid(a) | ||
|
|
There was a problem hiding this comment.
We were looking at this in the triage meeting as well. I guess the test is great (I honestly still need to parse it fully).
But we are missing an additional test for the successful path that was fixed I think: Checking that the array was indeed modified in-place with copy=False?
There was a problem hiding this comment.
I have added a new test in test_masked_array_no_copy() ... just to explain myself: I didn't add it in the first place because mask_invalid becomes a straight call of masked_where which is already tested. But I guess the more tests we have the better?
Thanks!
There was a problem hiding this comment.
Yeah, thanks. It is true that masked_where is tested, but it is also just nice to see the fix in action in the PR and we mainly have integration tests anyway.
A test too much cannot hurt :).
I find it odd that we consider inf invalid by default, but that is not a change.
Thanks @cmarmo, lets get this in!
071082b to
4213779
Compare
|
I suppose we should do this just to align things anyway... Although, now I actually suspect that this may have been intentional (Chesterton's fence greets): The function always replaces the mask, but does not always copy the data, which is a pattern to the madness that does make sense, the docs are probably just fuzzy on that intention (if it was the intention). |
|
@mhvk Do you have any thoughts on this? |
|
Not really. Overall, to me this PR makes sense: do as the doc states and just call |
|
Thanks @cmarmo and Marten, lets give this a shot then! |
|
Should probably add a release note for this. |
|
Hmmm, maybe better. @cmarmo are you interested in having a look at that, the instructions are in Otherwise, hopefully I will remember to follow up and do it :). |
|
Hmm, if the label is used consistently, a script could help to look for missing release notes before release time? |
I can take care of that if it can wait until the week-end. :) |
This pull request add the changelog for #22046.
This is the minimal solution to fix numpygh-22826 with as little change as possible. We should fix `getdata()` but I don't want to do that in a bug-fix release really. IMO the alternative is to revert numpygh-22046 which would also revert the behavior noticed in numpygh-22720 (which seems less harmful though). Closes numpygh-22826
This is the minimal solution to fix numpygh-22826 with as little change as possible. We should fix `getdata()` but I don't want to do that in a bug-fix release really. IMO the alternative is to revert numpygh-22046 which would also revert the behavior noticed in numpygh-22720 (which seems less harmful though). Closes numpygh-22826
This pull requests makes
mask_invalidconsistent withmask_wherewhencopyis set toFalse.Fixes #19332.
I'd rather make the two functions consistent than change the documentation.
Also from the documentation
I added a test checking that an error is thrown when
isinfiniteis not applicable.Thanks for considering it.