BUG: MaskedArray with bytes dtype cannot be compared if fill_value is set

[mhvk]

Describe the issue:

Astropy is having a number of test failures as a result of #21041. Some are just due to the new FutureWarning, which are fine. But some are due to something rather strange - that MaskedArray comparisons depend on whether fill_value is set.

Reproduce the code example:

import numpy as np
ma = np.ma.MaskedArray(['a', 'b'])
ma > ma
# masked_array(data=[False, False],
#              mask=False,
#        fill_value=True)

ma
# masked_array(data=['a', 'b'],
#              mask=False,
#        fill_value='N/A',
#             dtype='<U1')

ma > ma
# ---------------------------------------------------------------------------
# TypeError                                 Traceback (most recent call last)
# Input In [6], in <cell line: 1>()
# ----> 1 ma > ma

# TypeError: '>' not supported between instances of 'MaskedArray' and 'MaskedArray'

The real problem in the above is that showing `ma` actually sets its `_fill_value` and that somehow causes problems. (Explicitly setting `._fill_value` also gives a `TypeError` - and in `astropy` it is explicitly set.)

ma._fill_value = None
ma > ma

masked_array(data=[False, False],

mask=False,

fill_value=True)

Error message:

No response

NumPy/Python version information:

1.24.0.dev0+185.g4cb688938 3.10.5 (main, Jun 8 2022, 09:26:22) [GCC 11.3.0]

[charris]

Perhaps we should just back out #21041 for 1.23. It was a bit of a rush to backport it for the Numba folks and that may have been too optimistic.

[seberg]

Sorry, I had not realized there was so much oddity digging into those failures :(!

There are two main things going on:

1. We incorrectly silence the following error. This is because I missed to update the "error fixup" code to know that strings now go through the ufunc. The error is the following:

File ~/forks/numpy/build/testenv/lib/python3.9/site-packages/numpy/ma/core.py:3061, in MaskedArray.__array_finalize__(self, obj)
   3059 # Finalize the fill_value
   3060 if self._fill_value is not None:
-> 3061     self._fill_value = _check_fill_value(self._fill_value, self.dtype)
   3062 elif self.dtype.names is not None:
   3063     # Finalize the default fill_value for structured arrays
   3064     self._fill_value = _check_fill_value(None, self.dtype)

File ~/forks/numpy/build/testenv/lib/python3.9/site-packages/numpy/ma/core.py:467, in _check_fill_value(fill_value, ndtype)
    462         except (OverflowError, ValueError) as e:
    463             # Raise TypeError instead of OverflowError or ValueError.
    464             # OverflowError is seldom used, and the real problem here is
    465             # that the passed fill_value is not compatible with the ndtype.
    466             err_msg = "Cannot convert fill_value %s to dtype %s"
--> 467             raise TypeError(err_msg % (fill_value, ndtype)) from e
    468 return np.array(fill_value)

(may be pretty harmless on its own, but makes this a lot more confusing!)

2. In the above traceback we see __array_finalize__ now springing into action due to using the ufunc np.equal. That code actually seems to use self._fill_value. The fun part is that self._fill_value can be None, but if it is, it is automatically filled in when self.fill_value is used.
   The first time around, it is None and things work. The second time around it is not None and the fill value is not compatible with the ufunc result.
   (This likely indicates more subtle bugs, but...)

Now, that fill-value doesn't make sense for ufuncs that return a boolean output probably.

There is at least one more change:

• The old code did not check for RICHCMP_GIVE_UP_IF_NEEDED. This code is not affected by that, but I am somewhat worried that might have subtle consequences.
• Especially, because MaskedArray does only implement == and != but not the other comparisons.

---

My preference is that for the main branch, we aim for fixing the two issues:

1. Fixup the "error sanitizing" so that the above error is raised.
2. Add __le__, etc. to MaskedArray, because that will handle these paths (and any binop deferring) correctly.

For 1.23, I guess revering is the way to go. Fixing this seems like it would require both of the above fixes, and after that is gone there is still a tiny chance of subtleties around RICHCMP_GIVE_UP_IF_NEEDED

[seberg]

Actually, maybe the error sanitizing mainly toggles between FutureWarning and DeprecationWarning that might just cause subtle differences.
(And I am not sure yet which one is actually better; but I need to read the code closer probably.)

So it might just be the second one (plus another fixup) on main.

[seberg]

Now I am wondering if we should just add a special case to (I clearly should sleep on this :)):

numpy/numpy/ma/core.py

Lines 453 to 465 in c2db91c

	if isinstance(fill_value, str) and (ndtype.char not in 'OSVU'):
	# Note this check doesn't work if fill_value is not a scalar
	err_msg = "Cannot set fill value of string with array of dtype %s"
	raise TypeError(err_msg % ndtype)
	else:
	# In case we want to convert 1e20 to int.
	# Also in case of converting string arrays.
	try:
	fill_value = np.array(fill_value, copy=False, dtype=ndtype)
	except (OverflowError, ValueError) as e:
	# Raise TypeError instead of OverflowError or ValueError.
	# OverflowError is seldom used, and the real problem here is
	# that the passed fill_value is not compatible with the ndtype.

But of course, this becomes an obstacle course. The fill value is:

array('N/A', dtype='<U3')

and the reason for the failure is that:

>>> np.array(np.array('N/A', dtype='<U3'), dtype=bool)
ValueError: invalid literal for int() with base 10: 'N/A'

(just like casting) fails because it wants to interpret it as a number to cast...

So in the sense, the very long path to why this failure even occurs, is that our string to bool cast is still arguably broken :).

[pllim]

I cannot tell if this bug also causes all these other failures at https://github.com/astropy/astropy/runs/6918680788?check_suite_focus=true but the traceback suggests something about ufunc . If some are caused by other bugs, I can open new issues. Please let me know. Thanks!

xref astropy/astropy#13332 (comment)

p.s. We also see failures downstream of astropy that uses astropy.units, so this bug has far-reaching consequences for Astropy Project. E.g., see https://github.com/spacetelescope/jdaviz/runs/6919891933?check_suite_focus=true

[seberg]

Oh wow, I thought I had even run the astropy test-suite, but maybe the RuntimeWarnings were just warnings then.

I suspect all (or almost all) of them are related to gh-21437. That PR makes sure that we add warnings for all cast operations, that means basically things like:

np.array([np.nan]).astype(int)  # will give an "invalid value" RuntimeWarnig.
np.array([3e300]).astype(np.float32)  # will give an "overflow" RuntimeWarning

If the warning is raised, maybe the unit machinery will then think that a conversion is invalid (rather than overflowing?).

Lets continue on the astropy issue (don't hesitate to ping me there) or a new issue?

[seberg]

NOTE: I realize there is a bit of a "double" influx, but this change has of course nothing to do with the upcoming NumPy 1.23.

[charris]

I've reverted the #21041 backport for 1.23.0, see #21777.

[mhvk]

Just to confirm that these new bugs are unrelated to 1.23 and to this issue.

[mhvk]

@seberg - nice sleuthing on the issue. Though why is this now triggered and not before?

[seberg]

@mhvk ah, sorry... because before __lt__ etc. were not implemented, and the ndarray.__lt__ was not subclass aware:

In [2]: import numpy as np

In [3]: ma = np.ma.MaskedArray(["a", "b"])

In [4]: ma > ma
Out[4]: array([False, False])

[seberg]

I suppose the one "fix" short of reverting would be to convert the input arrays to bare NumPy arrays ahead of calling into np.equal, etc. (when the input is "string"). But, I am not really convinced that it is worth it.

[mhvk]

It sounds to me like it is best to hash this out for 1.24...