-
Notifications
You must be signed in to change notification settings - Fork 34
TST: add test that wrapping preserves view/copy semantics #333
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
base: main
Are you sure you want to change the base?
Conversation
204a42f
to
35c2d67
Compare
Okay, CI is green now across numpy from 1.22 to -dev. Will likely merge gh-334 first to separate the CI tweaks from -compat tweaks (currently, this PR contains both). |
What were other "unpleasantly surprising" cases, if you happen to remember @crusaderky ? |
there's the table at #331 (comment); there's |
Views are not a problem per se; returning a view when a bare library returns a copy is. |
|
Cannot repro, what am I missing?
|
These functions could return a view. They don't return a view in NumPy. I did not test the other backends wrapped by array-api-compat. |
Thanks for the clarification! So it's a theoretical concern at this point. IMO the reasonable thing to declare is #331 (comment) |
array_api_compat/common/_aliases.py
Outdated
if result.dtype != x.dtype: | ||
# numpy < 2: ceil(int array) is float | ||
result = xp.asarray(result, dtype=x.dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads to loss of precision for very large ints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks. Reworked to avoid round-tripping int->float->int.
…ry functions If a bare library returns a copy, so does the wrapped library; if the bare library returns a view, so does the wrapped library.
Remove these functions from common/_aliases.py, add specific implementations for numpy < 2 and cupy.
Co-authored-by: Guido Imperiale <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a cosmetic nit
def ceil(x: Array, /) -> Array: | ||
if cp.issubdtype(x.dtype, cp.integer): | ||
return x.copy() | ||
return cp.ceil(x) | ||
|
||
|
||
def floor(x: Array, /) -> Array: | ||
if cp.issubdtype(x.dtype, cp.integer): | ||
return x.copy() | ||
return cp.floor(x) | ||
|
||
|
||
def trunc(x: Array, /) -> Array: | ||
if cp.issubdtype(x.dtype, cp.integer): | ||
return x.copy() | ||
return cp.trunc(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int arguments are outside of the scope of the Array API.
So I'm not sure this should be here at all?
For numpy is a bit more nuanced - on one hand it makes sense to unify numpy 1.x behaviour to match 2.x.
On the other hand it's a array-api-compat tweak specifically to cover a change in behaviour that is already outside of the Array API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just backwards compat. On main, all backends try to return integers for integer inputs (via common/_aliases.py
), so this PR tries to preserve that, while fixing the views/copies issue.
Co-authored-by: Guido Imperiale <[email protected]>
If a bare library returns a copy, so does the wrapped library; if the bare library returns a view, so does the wrapped library.
Unary functions only for now.
cross-ref gh-331