BUG: Fix np.testing utils failing for masked scalar vs. scalar (#29317) #29318
Conversation
The two added test cases fail with: E AssertionError: E Arrays are not equal E E nan location mismatch: E ACTUAL: MaskedArray(3.) E DESIRED: array(3.) and E AssertionError: E Arrays are not equal E E nan location mismatch: E ACTUAL: MaskedArray(3.) E DESIRED: array(nan)
…#29317) TestArrayEqual.test_masked_scalar now passes. This case regressed since 7315145 (merged in numpy#12119) due to: - `<masked scalar> == <scalar>` returning np.ma.masked (not a 0-dim masked bool array), followed by - `np.bool(np.ma.masked)` unintentionally converting it to np._False There are a few ways to resolve this; I went with testing the comparison result with `isinstance(bool)` to check if a conversion to array is necessary, which is the same approach already taken in assert_array_compare after evaluating `comparison(x, y)`.
Confusingly, "isinstance(..., bool) checks" in the previous wording actually incorrectly referred to the ones towards the end of the function, which are not actually related to __eq__'s behavior but to the possibility of `func` returning a bool.
- Use same language as elsewhere below to explain `!= True` used to handle np.ma.masked - Clarify committed to support standard MaskedArrays - Restore note lost in 7315145 comment changes about how the np.bool casts towards the end of the function handle np.ma.masked, and expand further.
|
What is currently broken? What does this PR change? I think the reason this PR hasn't been reviewed yet is because it has a blank description and does nontrivial things that aren't obvious just from the PR title. |
Described in the linked bug:
The title says that it fixes the bug though... I moved a later comment "Resolves #29317" into the description, if that's helpful. |
I don't think the I'm not sure it makes sense to do one-off fixes like this, especially this one that does a cast based on the result of an isinstance call, which feels pretty brittle to me. Much better IMO to add e.g. a new np.ma.testing module that intentionally accepts masked arrays and handles all these cases, just like we have for all the other masked operations. Sadly masked arrays are somewhat of an unloved feature no one as far as I'm aware it working on any systematic fixes for them like this. |
|
Thanks for the reference to #14624. Personally, I agree it makes more sense to have a separate I've made these one-off changes not for their own sake, but as incremental fixes towards #28827. I'd described that in some more detail under the Context section of #29317:
|
|
ping @mhvk since you merged a fix a while back along these lines - what do you think? |
| # support them if possible. | ||
| if np.bool(x_id == y_id).all() != True: | ||
| result = x_id == y_id | ||
| if isinstance(result, bool): |
There was a problem hiding this comment.
This feels pretty brittle to me. Is there another way to do this without isinstance?
There was a problem hiding this comment.
There are other ways, but have you seen the rationale in the commit message for d1963d9 ?
There are a few ways to resolve this; I went with testing the
comparison result withisinstance(bool)to check if a conversion to
array is necessary, which is the same approach already taken in
assert_array_compare after evaluatingcomparison(x, y).
There was a problem hiding this comment.
The difference here is that logic has been there a long time, so people have written code to handle it. In this case, you're introducing a possibility for NumPy to suddently generate AttributeErrors in downstream tests when NumPy didn't do that before.
| result = x_id == y_id | ||
| if isinstance(result, bool): | ||
| result = np.bool(result) | ||
| if result.all() != True: |
There was a problem hiding this comment.
Playing devil's advocate: what if someone passes something into this routine that returns something that's not a bool from == but that also doesn't have an all() method. You've introduced a new AttributeError that the cast to np.bool used to guard against.
Resolves #29317
Fixes
np.testing.assert_XXXutilities failing when comparing a masked scalar against another scalar, with a "nan location mismatch" error. The consistent behavior is to ignore any masked elements, in this case the only one, and therefore succeed.See also #11121