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

Add __array_ufunc__ and __array__function__ to ArrayIndex #96

Merged

Conversation

telamonian
Copy link
Contributor

Fixes #95

Adds appropriate definitions for the __array_ufunc__ and __array__function__ to the ArrayIndex class. This makes it so that if a is a ndarray and b is a ArrayIndex, both a == b and b == a get dispatched to b.__eq__(a)

@asmeurer
Copy link
Member

You should update the == tests in test_ndindex.py.

What are the other consequences of this? I don't want array index types to be treatable as arrays (see https://quansight.github.io/ndindex/type-confusion.html#integerarray-and-booleanarray). Array operations should in general not work on them.

@asmeurer
Copy link
Member

Oh I didn't see your comment on the issue. Given that, this does seem like the best option.

@asmeurer
Copy link
Member

Also I just merged #93 which updates that test, so you'll need to update this on top of that.

ndindex/array.py Outdated
@@ -14,6 +14,10 @@ class ArrayIndex(NDIndex):
To subclass this, define the `dtype` attribute, as well as all the usual
ndindex methods.
"""
__array_ufunc__ = None
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment here explaining why these are here.

@asmeurer
Copy link
Member

This looks good. I would just like a comment explaining what this does. I guess it's more possible to override NumPy behavior than I thought. Now if only NumPy would allow a[idx] to be overridden so that we don't have to use a[idx.raw] for ndindex types.

@telamonian
Copy link
Contributor Author

There's probably a NEP for that. Turns out there's a NEP for everything :)

@asmeurer
Copy link
Member

I wonder if we should also at least define

def __array__(self):
    return self.array

Right now we have

>>> a = np.array([True, False])
>>> idx = ndindex(a)
>>> np.array(idx)
array(BooleanArray([True, False]), dtype=object)

We could also make it give an error.

    def __array__(self):
        raise TypeError(f"Cannot convert {self.__class__.__name__} to an array. Use .array instead.")

I'm not sure which way is better.

@telamonian
Copy link
Contributor Author

ArrayIndex instances are not arrays. So they shouldn't just transparently cast to arrays.

They're hardly rules to live by, but I've found the zen of python to be good guidelines for building the simplest, most intuitable version of complex behaviors. Here I think the relevant parts are

"Explicit is better than implicit."

and

"There should be one-- and preferably only one --obvious way to do it."

So I think your second suggestion about raising a TypeError is good. But I'd go even further and say that the .array prop is superfluous and potentially confusing, and should be removed in favor of just .raw

@telamonian
Copy link
Contributor Author

telamonian commented Oct 25, 2020

This looks good. I would just like a comment explaining what this does. I guess it's more possible to override NumPy behavior than I thought. Now if only NumPy would allow a[idx] to be overridden so that we don't have to use a[idx.raw] for ndindex types.

Honestly this does seem like a good idea. Maybe we really should consider opening a NEP about ndarray.__getitem__ dispatch. I opened an issue for this at #97

@asmeurer
Copy link
Member

asmeurer commented Dec 9, 2020

Sorry for not looking into your PRs. The CI was broken on ndindex because of an upstream numpy issue, but that's been resolved now that the 1.20 prerelease is out. I've pushed a commit here addressing my comment, and I will merge once the tests pass.

@asmeurer asmeurer merged commit be53243 into Quansight-Labs:master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ndarray(x) == NDIndex(x) and NDIndex(x) == ndarray(x) should return a consistent result
3 participants