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: MaskedArray does not seem to respect ufunc dispatch hierarchy #15200

Open
jthielen opened this issue Dec 30, 2019 · 4 comments · Fixed by #16022 or #21977 · May be fixed by #22914
Open

BUG: MaskedArray does not seem to respect ufunc dispatch hierarchy #15200

jthielen opened this issue Dec 30, 2019 · 4 comments · Fixed by #16022 or #21977 · May be fixed by #22914
Labels
component: numpy.ma masked arrays

Comments

@jthielen
Copy link

When using a custom array container that implements __array_ufunc__ and is meant to be able to wrap MaskedArrays, non-commutativity occurs in binary operations with MaskedArrays. Despite what the below comment mentions:

https://github.com/numpy/numpy/blob/v1.17.3/numpy/ma/core.py#L3960-L3972

it does not properly defer, as seen in the example below. I am unfortunately not well-acquainted with the MaskedArray internals, so I don't know what would be a good way forward for a fix.

xref hgrecco/pint#633

Reproducing code example:

import numpy as np
import numpy.lib.mixins


class WrappedArray(numpy.lib.mixins.NDArrayOperatorsMixin):
    __array_priority__ = 20

    def __init__(self, array, **attrs):
        self._array = array
        self.attrs = attrs

    def __repr__(self):
        return f"{self.__class__.__name__}(\n{self._array}\n{self.attrs}\n)"

    def __array__(self):
        return np.asarray(self._array)

    def __array_ufunc__(self, ufunc, method, *inputs, **kwargs):
        if method == '__call__':
            inputs = [arg._array if isinstance(arg, self.__class__) else arg
                      for arg in inputs]
            return self.__class__(ufunc(*inputs, **kwargs), **self.attrs)
        else:
            return NotImplemented


# Show basic wrap
w = WrappedArray(np.arange(3), test=1)
print(w)
print()

# Wrapping MaskedArrays works fine
wm = WrappedArray(np.ma.masked_array([1, 3, 5], mask=[False, True, False]),
                  test=2)
print(wm)
print()

# Operations with ndarrays work fine
a = np.array([2, 0, 1])
print(a * w)
print()

# Operations with masked arrays are not commutative
m = np.ma.masked_array([2, 0, 1], mask=[False, True, False])

# Good
print(w * m)
print()

# Bad
print(m * w)

Output:

WrappedArray(
[0 1 2]
{'test': 1}
)

WrappedArray(
[1 -- 5]
{'test': 2}
)

WrappedArray(
[0 0 2]
{'test': 1}
)

WrappedArray(
[0 -- 2]
{'test': 1}
)

[0 -- 2]

Numpy/Python version information:

1.17.3 3.6.7 | packaged by conda-forge | (default, Nov 6 2019, 16:19:42)
[GCC 7.3.0]

bors bot added a commit to hgrecco/pint that referenced this issue Dec 30, 2019
963: Add tests and documentation with improvement of downcast type compatibility (part of #845) r=hgrecco a=jthielen

As a part of #845, this PR adds tests for downcast type compatibility with Sparse's `COO` and NumPy's `MaskedArray`, along with more careful handling of downcast types throughout the library. Also included is new documentation on array type compatibility, including the type casting hierarchy digraph by @shoyer and @crusaderky.

While this PR doesn't fully bring Pint's downcast type compatibility to a completed state, I think this gets it "good enough" for the upcoming release, and the remaining issues are fairly well defined:

- MaskedArray non-commutativity (#633 / numpy/numpy#15200)
- Dask compatibility (#883)
- Addition of CuPy tests (no issue on issue tracker yet)

Because of that, I think this can close #845, but if @hgrecco you want that kept open until the above items are resolved, let me know.

- [x] Closes #37; Closes #845
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- [x] The change is fully covered by automated unit tests
- [x] Documented in docs/ as appropriate
- [x] Added an entry to the CHANGES file


Co-authored-by: Jon Thielen <[email protected]>
@shoyer
Copy link
Member

shoyer commented Dec 30, 2019

ping @mhvk who may have some thoughts here.

I suspect the right fix is to define a __array_ufunc__ method on MaskedArray, and rewrite the arithmetic special methods from:

   def add(self, other):
        if self._delegate_binop(other):
            return NotImplemented
        return add(self, other)

to

   def add(self, other):
        if self._delegate_binop(other):
            return NotImplemented
        return np.add(self, other)

Currently MaskedArray overrides ufuncs via __array_finalize__, which is quite indirect. There is an __array_ufunc__ method defined, but only via inheritance from the base np.ndarray class.

bors bot added a commit to hgrecco/pint that referenced this issue Dec 31, 2019
966: Update documentation to address MaskedArray breaking change r=hgrecco a=jthielen

As discussed in #963 (comment), #963 introduced a breaking change with MaskedArrays with the implementation of `__array_ufunc__` on Unit. This PR documents this breaking change as prompted by #963 (comment) (with the hope that numpy/numpy#15200 can be resolved soon!)

- ~~Closes # (insert issue number)~~
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- ~~The change is fully covered by automated unit tests~~ (documentation update only)
- [x] Documented in docs/ as appropriate
- [x] Added an entry to the CHANGES file


Co-authored-by: Jon Thielen <[email protected]>
@mhvk
Copy link
Contributor

mhvk commented Jan 3, 2020

Yes, MaskedArray should gain a proper __array_ufunc__... Right now, you also have to use the "old" system of overrides, and write a custom __array_wrap__. I think this may be do-able, but obviously less clear if it is worth the effort.

@rgommers rgommers added the component: numpy.ma masked arrays label Apr 18, 2020
greglucas added a commit to greglucas/numpy that referenced this issue Apr 14, 2021
This test makes sure that a MaskedArray defers properly to another
class if it doesn't know how to handle it. See numpy#15200.
mattip pushed a commit that referenced this issue Jul 9, 2022
This test makes sure that a MaskedArray defers properly to another
class if it doesn't know how to handle it. See #15200.
@dopplershift
Copy link

This issue should be re-opened since #16022 was reverted.

@seberg seberg reopened this Jul 11, 2022
NamamiShanker pushed a commit to NamamiShanker/numpy that referenced this issue Jul 13, 2022
This test makes sure that a MaskedArray defers properly to another
class if it doesn't know how to handle it. See numpy#15200.
greglucas added a commit to greglucas/numpy that referenced this issue Jul 13, 2022
This test makes sure that a MaskedArray defers properly to another
class if it doesn't know how to handle it. See numpy#15200.
greglucas added a commit to greglucas/numpy that referenced this issue Jul 13, 2022
This test makes sure that a MaskedArray defers properly to another
class if it doesn't know how to handle it. See numpy#15200.
evinism pushed a commit to evinism/numpy that referenced this issue Jul 17, 2022
This test makes sure that a MaskedArray defers properly to another
class if it doesn't know how to handle it. See numpy#15200.
@mattip
Copy link
Member

mattip commented Jul 20, 2022

Reopening, since the PR to use __array_ufunc__ was reverted.

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