Skip to content

BUG: MaskedArray ignores __array_priority__ #5230

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

Closed
wants to merge 2 commits into from

Conversation

abalkin
Copy link
Contributor

@abalkin abalkin commented Oct 24, 2014

fixes: #5227

This patch partially fixes the issue (I only covered addition, subtraction and multiplication). If you agree that this is the right approach - I will add remaining operations.

# Python 2.6 compatibility
if not hasattr(TestCase, 'assertIsInstance'):
def assertIsInstance(self, obj, cls):
self.assertTrue(isinstance(obj, cls)
Copy link
Member

Choose a reason for hiding this comment

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

Missing ).

Copy link
Member

Choose a reason for hiding this comment

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

Causes universal test failures.

@charris charris added this to the 1.10.0 release milestone Apr 7, 2015
@charris
Copy link
Member

charris commented Apr 26, 2015

@abalkin Could you finish this up?

self.__doc__ = getattr(mbfunc, "__doc__", str(mbfunc))
self.__name__ = getattr(mbfunc, "__name__", str(mbfunc))
ufunc_domain[mbfunc] = None
ufunc_fills[mbfunc] = (fillx, filly)

def __get__(self, obj, type=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly confused why this is being done. It makes this a descriptor, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is this what allows you to directly define __add__ below?

@mhvk
Copy link
Contributor

mhvk commented May 6, 2015

@abalkin - pointed to this by @charris -- this seems a good approach, and is probably the safest one can be.

In a slightly larger picture, though, I wonder if one isn't duplicating more of the ufunc machinery than necessary. I somewhat wonder why all the methods are overridden at all; with __mul__ removed, the normal code would return NotImplemented and pass on to X.__rmul__. One can see this by using a function that is not defined, such as __or__ (lucky some where missed!):

class X(int):
    __array_priority__ = 1000
    def __ror__(self, other):
        return 42

np.ma.array(1) | X(2)
# yields 42

Anyway, this mostly as a curiosity; I fear removing the methods would imply a great deal more work, and possibly a lot of rewriting of __array_wrap__ -- which would suggest one might as well go for __numpy_ufunc__ (which should make MaskedArray much simpler).

@abalkin
Copy link
Contributor Author

abalkin commented May 9, 2015

@mhvk - I agree that __numpy_ufunc__ is the solution, but I am not sure it is ready for prime time at this point. On the other hand rewriting ma to use __numpy_ufunc__ may serve as a good test-bed.

@mhvk
Copy link
Contributor

mhvk commented May 9, 2015

@abalkin - from my work on Quantity, I think __numpy_ufunc__ as currently implemented should do the trick; I don't think anything that is decided in #5844 would change that. That said, it would be quite a lot of work, and one of those cases where it is unclear whether one should start changing the code or just start from scratch and just ensure the new code passes all tests (which I think are decently complete for MaskedArray). I know I won't have time to do this myself anytime soon, but I'd be willing to help, both in thinking of approaches and in testing that it resolves the issues I have raised with the current implementation (#3907, #4586, #4617)

If you want to have some sense of what a __numpy_ufunc__ method would look like, see astropy/astropy#2948 and in particular the code starting at https://github.com/mhvk/astropy/blob/quantity-numpy-ufunc-additions/astropy/units/quantity.py#L409

@charris
Copy link
Member

charris commented May 13, 2015

Is the suggestion that I can close this in favor of a __numpy_ufunc__ approach at some future time? I confess that adding an op argument to standard python functons strikes me a bit odd.

@njsmith
Copy link
Member

njsmith commented May 13, 2015

I believe that #5864 also addresses the issues here in a different way.
On May 13, 2015 1:03 PM, "Charles Harris" notifications@github.com wrote:

Is the suggestion that I can close this in favor of a numpy_ufunc
approach at some future time? I confess that adding an op argument to
standard python functons strikes me a bit odd.


Reply to this email directly or view it on GitHub
#5230 (comment).

@mhvk
Copy link
Contributor

mhvk commented May 13, 2015

Specifically, njsmith@c3c819a
@abalkin - does that commit indeed resolve your problems as well?

@charris
Copy link
Member

charris commented May 13, 2015

I'm going to close this, if someone wants to pursue this, please squeak.

@njsmith A short explanation of how #5864 would solve this would be useful.

@charris charris closed this May 13, 2015
@njsmith
Copy link
Member

njsmith commented May 13, 2015

Currently there are two different mechanisms used to handle the interaction
of array_priority and binop dispatch: ndarray uses one (in
number.c), and np.ma uses the other (in umath). They work differently in
subtly ways, and this bug is about one of the places they disagree. The
main goal of #5864 is to clean up the ufunc code, so I had to switch np.ma
to stop using the ufunc stuff. Since I had to redo it anyway, I switched
np.ma to use the same array_priority rule that ndarray already uses,
which has the side-effect of fixing this issue.
On May 13, 2015 2:32 PM, "Charles Harris" notifications@github.com wrote:

I'm going to close this, if someone wants to pursue this, please squeak.

@njsmith https://github.com/njsmith A short explanation of how #5864
#5864 would solve this would be
useful.


Reply to this email directly or view it on GitHub
#5230 (comment).

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.

MaskedArray ignores __array_priority__
5 participants