-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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: numpy.ma: Fix ma.in1d
#20011
base: main
Are you sure you want to change the base?
BUG: numpy.ma: Fix ma.in1d
#20011
Conversation
Hi @tirthasheshpatel we discussed this at the last triage meeting and would like to review/merge. Could you rebase off main in order to pick up any CI changes that might have happened since this was first submitted? |
8963eb4
to
b534b6b
Compare
Great, thanks!
Done. |
Seems to make sense to me. @WarrenWeckesser want to take a look? |
out = np.in1d(ar1, ar2, assume_unique=assume_unique, | ||
invert=invert) | ||
if m is not nomask: | ||
res[~m] = out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the output array be masked by the intersection of the mask of ar1 and ar2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this function, the mask of the output array is determined by the mask of the first argument ar1
only. This is not a pairwise operation; the mask of the second argument actually has no effect on the output.
ar1, ar2 = ma.asarray(ar1), ma.asarray(ar2) | ||
m = getmask(ar1) | ||
res = ma.empty_like(ar1, dtype='bool') | ||
if ar1.mask is not nomask: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have ar1.mask
in m
, so here and in the next line, use m
instead of ar1.mask
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and if you prefer the readability of ar1.mask
, you could rename m
to ar1_mask
.
It looks like this changed the behavior of
But in this branch, we get an exception:
|
Fixes #19877
Fixed
ma.in1d
and add tests cases from gh-19877.I just rewrote the function to use
numpy.in1d
instead of reinventing the wheel for masked arrays. To me, this seems more maintainable. I don't know if there is any reason to handle masked arrays differently. If so, sorry; I'll close this PR.