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: Fix fill value in masked array '==' and '!=' ops. #12257

Merged
merged 2 commits into from
Oct 26, 2018

Conversation

charris
Copy link
Member

@charris charris commented Oct 24, 2018

The type of the fill_value needs to be bool_ in order to match
the result type of == and !=.

Closes #12248.

The type of the fill_value needs to be `bool_` in order to match
the result type of `==` and `!=`.

Closes numpy#12248.
@eric-wieser
Copy link
Member

Would it make sense for (a == b).fill_value to be a.fill_value == b.fill_value, rather than force_bool_conversion(a.fill_value)?

@charris
Copy link
Member Author

charris commented Oct 24, 2018

@eric-wieser That's a whole other topic which affects other binary operators. Currently, IIRC, the fill value for other binary operators is taken from the first argument if not None, otherwise from of the second, which I suppose we could extend to the == and != operators. I was just updating the current behavior that takes it from self and adding a conversion to bool_ as the minimal change needed to make a fix. I don't think a.fill_value == b.fill_value makes much sense here.

@charris
Copy link
Member Author

charris commented Oct 24, 2018

@eric-wieser I'm actually not convinced that instances of masked arrays really need a fill value, seems more like something that should be put in at the function call level.

Cover the string and numeric types. Structured types were
already covered.
Copy link
Contributor

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

I just wanted to confirm that this fixes the issues we were seeing in astropy - thanks @charris!

@eric-wieser
Copy link
Member

I don't think a.fill_value == b.fill_value makes much sense here.

The rationale would be that (a == b).filled() should aim to be the same as a.filled() == b.filled()

@eric-wieser
Copy link
Member

Does this manifest itself for non-structured arrays? From the test, it seems you only test structured ones

@charris
Copy link
Member Author

charris commented Oct 25, 2018

Does this manifest itself for non-structured arrays? From the test, it seems you only test structured one

?. I only added tests for non-structured dtypes. There were some previous tests for structured dtypes.

The rationale would be that (a == b).filled() should aim to be the same as a.filled() == b.filled()

Still don't see the use, as a.filled() == b.filled() doesn't actually appear anywhere, and the result is probably accidental in any case.

stevereyes01 pushed a commit to stevereyes01/pycbc that referenced this pull request Oct 25, 2018
@charris charris merged commit 3debe97 into numpy:master Oct 26, 2018
@charris charris deleted the fix-ma-compare-fill_value branch October 26, 2018 14:35
@astrofrog
Copy link
Contributor

@charris - just to check, do you have an ETA for the 1.15.4 release? We're going to do a major Astropy release shortly (within <1 week) so we're trying to figure out whether we should add workarounds to Astropy (but we won't if 1.15.4 is out first). Thanks!

@charris
Copy link
Member Author

charris commented Oct 30, 2018

@astrofrog I was just thinking about that. Normally, I'd make it about the 20'th, but given that 1.15 is at end of releases and pretty much only needs this fix, I was thinking maybe this next weekend. I'd be happy to coordinate the release with astropy.

@astrofrog
Copy link
Contributor

@charris - this next weekend would be perfect and in time for the astropy release, if you are able to do that. We'll be issuing a release candidate this week and the final release next week - and this is the one for which we'll need to make sure numpy 1.15.4 is release to avoid users running into issues. Thanks!

@charris
Copy link
Member Author

charris commented Oct 30, 2018

OK, will do.

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.

None yet

3 participants