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

ENH: Add warning in median, percentile and quantile when array is MaskedArray #15180

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mproszewska
Copy link
Contributor

Closes #14716

@mproszewska mproszewska changed the title Mask warning ENH Add warning in median, percentile and quantile when array is MaskedArray Dec 26, 2019
@mproszewska mproszewska changed the title ENH Add warning in median, percentile and quantile when array is MaskedArray ENH: Add warning in median, percentile and quantile when array is MaskedArray Dec 26, 2019
@eric-wieser eric-wieser added the component: numpy.ma masked arrays label Dec 26, 2019
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

I'm not a huge fan of this approach - numpy.ma is supposed to be built on top of numpy, not integrated into it. Perhaps practicality beats purity here though.

numpy/lib/function_base.py Outdated Show resolved Hide resolved
@mattip
Copy link
Member

mattip commented Dec 26, 2019

Another approach would be for MaskedArray to use __array_function__ and override the calls with its own implementation

@eric-wieser
Copy link
Member

Even if that implementation does nothing but warn that the mask is not supported.

Problem is, __array_function__ is as I remember it all or nothing - we can't add it for one function without breaking everything else.

@mattip
Copy link
Member

mattip commented Dec 26, 2019

Problem is, array_function is as I remember it all or nothing

It dispatches on all functions, but we can defer on ones we do not wish to override. The example below this link shows that design pattern

@rgommers
Copy link
Member

Problem is, array_function is as I remember it all or nothing

It dispatches on all functions, but we can defer on ones we do not wish to override. The example below this link shows that design pattern

It doesn't, that returns NotImplemented which in turn will raise a TypeError in the __array_function__ machinery. It pretty much is all or nothing as @eric-wieser says. You could explicitly coerce the masked arrays to ndarrays, and then call the np.somefunc again, but that's a pretty invasive change for the purpose of adding a warning.

@charris
Copy link
Member

charris commented Dec 26, 2019

There was some discussion of making a new masked array implementation that would use the __array_function__ option. I think there was even a rough prototype.

@mattip
Copy link
Member

mattip commented Dec 26, 2019

@eric-wieser
Copy link
Member

This is one of those cases where the NotImplementedButCoercible proposal that @hameerabbasi was proposing would be useful. Another middle ground might be to have ndarray.__array_function__ allow subclasses, not just ndarray.

@mproszewska
Copy link
Contributor Author

mproszewska commented Jan 13, 2020

Probably the best option is to make a new masked array implementation as in prototype and add warnings/implementation there.
I agree that current approach in this PR is not the best, but I still think that there should be an information about unexpected behavior of median, percentile and quantile functions called on MaskedArray. Maybe just in documentation.
Should I remove warnings in this PR and add appropriate comments in docs?
Could I work on a new masked array implementation? If so, how can I do that?

Base automatically changed from master to main March 4, 2021 02:04
@InessaPawson InessaPawson added the 52 - Inactive Pending author response label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Awaiting a decision
Development

Successfully merging this pull request may close these issues.

np.quantile does not ignore masked values with masked_arrays
7 participants