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: reject ndarrays in binary operators #103

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ev-br
Copy link
Contributor

@ev-br ev-br commented Nov 27, 2024

A minimal fix for gh-102.
As a byproduct, add several check_device calls, where previously missing.

@asmeurer
Copy link
Member

Should add some tests for this.

@ev-br
Copy link
Contributor Author

ev-br commented Nov 27, 2024

I believe 6d67d46#diff-8047174d1f9158b619da8e809ed90fd46404caae67e7f687c5ba3a77f5bddc40R215 is a faithful representation of what we can test.

@asmeurer
Copy link
Member

Ah sorry, I didn't notice that last bit was actually already in the tests. That should be fine.

This change seems OK. It fixes some of the more egregious uses of __array__. Maybe we should also check scipy with this change too (should we add something like that to the CI?), since the last time things broke so spectacularly.

@ev-br
Copy link
Contributor Author

ev-br commented Nov 28, 2024

Indeed, scipy's $ python dev.py test -b array_api_strict passes locally with this PR.

W.r.t. CI testing of downstreams, yeah, this might be useful. An immediate downside though is that it'll make a CI run from under five minutes to > 1/2 hour. It is definitely worth thinking a bit about a larger CI strategy, here or in -compat. Maybe as a cron job or a manual trigger. I feel it should be a separate PR though.

@asmeurer
Copy link
Member

I would go with whatever's the most pragmatically useful for testing scipy. It would be a good idea to test it before doing releases at the very least, to avoid the issues we had previously.

@@ -234,6 +234,8 @@ def _check_device(self, other):
elif isinstance(other, Array):
if self.device != other.device:
raise ValueError(f"Arrays from two different devices ({self.device} and {other.device}) can not be combined.")
else:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should explicitly only reject ndarray. That way NotImplemented can still work on other types (although I honestly don't know how important that is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd think that mixing different array objects is a bad idea in general, and is basically impossible to make consistent. Even when it fails, there are just too many possible reasons or failure modes.
So one either buys in duck-typing all the way (and whatever quacking falls out, it does), or say no to anything which is not the same array namespace. If -strict strives to be strict, the latter makes sense, it seems.
That said, I'm happy with whatever you think is best.

In [4]: xp.arange(5) + cp.arange(5)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[4], line 1
----> 1 xp.arange(5) + cp.arange(5)

File cupy/_core/core.pyx:1265, in cupy._core.core._ndarray_base.__add__()

File cupy/_core/_kernel.pyx:1286, in cupy._core._kernel.ufunc.__call__()

File cupy/_core/_kernel.pyx:159, in cupy._core._kernel._preprocess_args()

File cupy/_core/_kernel.pyx:145, in cupy._core._kernel._preprocess_arg()

TypeError: Unsupported type <class 'array_api_strict._array_object.Array'>

In [5]: cp.arange(3) + xp.arange(3)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[5], line 1
----> 1 cp.arange(3) + xp.arange(3)

File cupy/_core/core.pyx:1269, in cupy._core.core._ndarray_base.__add__()

TypeError: operand type(s) all returned NotImplemented from __array_ufunc__(<ufunc 'add'>, '__call__', array([0, 1, 2]), Array([0, 1, 2], dtype=array_api_strict.int64)): 'ndarray', 'Array'

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.

2 participants