Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Define <, <=, >, >= for masked arrays #21812

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Conversation

seberg
Copy link
Member

@seberg seberg commented Jun 21, 2022

This defines the comparison operators (other than == and !=)
explicitly for masked arrays.
The mask is ignored for the resuling res._data (unlike == and
!= which take the mask into account.

Closes gh-21770, although the way that masked arrays propagate the
fill-value seems generally broken and error prone.

@seberg
Copy link
Member Author

seberg commented Jun 21, 2022

Arg, no... this should not really be necessary, closing for now.

@seberg seberg closed this Jun 21, 2022
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I agree with the actual underlying data values just being the result of the comparison.

@mhvk
Copy link
Contributor

mhvk commented Jun 21, 2022

Oops, I hadn't seen you closed it! Why would this not be necessary? It is true that in principle this could be done inside __array_ufunc__, but that would be a bigger effort... Or is it just that this is more than is needed to fix #14727?

@seberg
Copy link
Member Author

seberg commented Jun 21, 2022

Yeah, I will reopen (and remove the release notes, I don't think they are actually necessary).

@seberg seberg reopened this Jun 21, 2022
This defines the comparison operators (other than `==` and `!=`)
explicitly for masked arrays.
The mask is ignored for the resuling `res._data` (unlike `==` and
`!=` which take the mask into account.

Closes numpygh-21770, although the way that masked arrays propagate the
fill-value seems generally broken and error prone.
@seberg
Copy link
Member Author

seberg commented Jun 21, 2022

I guess my point was that the fill-value propagation seems broken for all ufuncs at least when the result dtype does not match the (first) input. The best solution is probably indeed __array_ufunc__ though (maybe not the only solution, but presumably much cleaner than the alternative).

EDIT: Maybe unclear... This is sufficient to fix all the issues. However, if ufuncs were not broken, this should not be necessary. For one, it leaves np.logical_or broken the same way.

@mhvk
Copy link
Contributor

mhvk commented Jun 21, 2022

Yes, agreed that fill-value propagation is broken/tricky. But separate from this PR at least.

@seberg seberg changed the title API: Define <, <=, >, >= for masked arrays BUG: Define <, <=, >, >= for masked arrays Jun 23, 2022
@seberg seberg added this to the 1.24.0 release milestone Jun 23, 2022
@charris
Copy link
Member

charris commented Jun 23, 2022

@seberg Are you happy with this?

@seberg
Copy link
Member Author

seberg commented Jun 23, 2022

Yes, happy enough andnd it should fix (most) of astropy's current issue. We still need to figure out if there is something to do about gh-21784 or not (but it is distinct).

@charris
Copy link
Member

charris commented Jun 23, 2022

Thanks Sebastian.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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