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.

Sign up for GitHub

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

Jump to bottom

Support the copy keyword in asarray #119

Merged
merged 41 commits into from
Mar 27, 2024
Merged

Support the copy keyword in asarray #119

merged 41 commits into from
Mar 27, 2024

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Mar 20, 2024 •
edited
Loading

Not finished yet. Still need to:

  • Make sure everything is tested on CI, including NumPy 1.21, 1.26, and 2.0
  • Implement CuPy and dask.array support.
  • Fix the device keyword in cupy.asarray (currently it is ignored!)

asmeurer added 6 commits March 19, 2024 13:05
@asmeurer
Fix some internal documentation
e75ba03
@asmeurer
Factor out the list of wrapped libraries in the tests
41719af 
This does remove jax testing from the isdtype test, since we should not test
jax functionality for non-helper functions.
@asmeurer
Fix typo
f1068a3
@asmeurer
Add a test for the copy flag in asarray
73047a9 
This test currently fails because this logic isn't implemented correctly for
numpy/cupy/dask. It does pass for pytorch.
@asmeurer
Properly test copy=None with the dtype argument
fa36c20
@asmeurer
Move the asarray numpy implementation to numpy/_aliases
558e4ed 
This now properly supports all relevant versions of NumPy.

This also removes the asarray_numpy function from the namespace.

I plan to do the same for cupy and dask as well.
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 20, 2024 22:04 — with GitHub Actions Inactive
asmeurer added 5 commits March 21, 2024 15:32
@asmeurer
Update xfails
440e1c1
@asmeurer
Add a CuPy specific implementation for asarray
f57ac54 
This also fixes the device keyword to actually work, and fixes copy=None
logic. copy=False is not implemented and requires upstream support, which
should come in CuPy 14.
@asmeurer
Update test to test that CuPy does not handle copy=False
166c650
@asmeurer
Update cupy buffer protocol copy test
a194344
@asmeurer
Merge branch 'main' into asarray-copy
a1eea09
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 21, 2024 22:12 — with GitHub Actions Inactive
asmeurer
asmeurer commented Mar 21, 2024
View reviewed changes
array_api_compat/cupy/_aliases.py Outdated
# cupy is like NumPy 1.26 (except without _CopyMode). See the comments
# in asarray in numpy/_aliases.py.
if copy is None:
copy = False
Copy link
Member Author

Choose a reason for hiding this comment

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

@leofang did you say at the meeting today that cupy 14 would definitely support NumPy 2.0? The meaning of copy=False changed between NumPy 1.26 and 2.0. Is it safe to add a if cupy.__version__ >= 14 check here? If we just leave this wrapper code as it is, it will break once cupy makes the change, because the default would become the array API copy=False semantics.

Copy link

Choose a reason for hiding this comment

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

It was TBD so here's my question: I would think like dpctl once CuPy is compliant we'd no longer need array-api-compat coverage? If so, I don't think we need a version check here, as it'd become moot anyway? WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way the compat library currently works is unconditionally wraps cupy. We could remove the wrapping once cupy is compliant but we'd need a version check to do that too.

Copy link

Choose a reason for hiding this comment

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

I mean, the version check could be added to, say, __init__.py instead of here, once we know better after which CuPy version all array-api-compat wrappers can just fall back to native CuPy without extra action (=no-ops), right? Maybe I oversimplified it?

Copy link
Member Author

Choose a reason for hiding this comment

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

If cupy is completely compliant and needs no wrappers we could do that. We're not doing that yet for numpy 2.0 but we probably should.

At any rate, I've thought of a better way to structure this code so that it won't actually break when cupy changes the meaning of copy=False, so this isn't actually important. I'll still need to update this to remove the NotImplementedError for copy=False, but is isn't pressing to try to do it ahead of time now because I've made it so that the default behavior won't break, and that would be a new feature for cupy anyways.

@asmeurer
Structure the copy flag in cupy.asarray better
d84983a 
This way it is more future-proof for when cupy changes the meaning of
copy=False.
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 22, 2024 07:43 — with GitHub Actions Inactive
asmeurer added 2 commits March 22, 2024 13:51
@asmeurer
Remove no longer correct note from docstring
11d27dd
@asmeurer
Add dask.array specific implementation of asarray()
f6b5ea2 
This also fixes several bugs in the implementation, and updates some tests.
@asmeurer
Copy link
Member Author

@lithomas1 can you take a look at the dask.array changes here.

@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 22, 2024 20:09 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 22, 2024 20:26 — with GitHub Actions Inactive
@asmeurer asmeurer marked this pull request as ready for review March 22, 2024 20:27
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 22, 2024 20:28 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 22, 2024 21:16 — with GitHub Actions Inactive
@asmeurer
Copy link
Member Author

Python 3.8 is starting to become a nuisance. I think I'm going to just drop support for it.

@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 20:31 — with GitHub Actions Inactive
@asmeurer
Only run numpy specific tests for numpy=dev
f7fb29f 
Other libraries currently break with numpy 2.0.
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 20:41 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 21:00 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 21:05 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 21:10 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 21:12 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 21:20 — with GitHub Actions Inactive
@asmeurer asmeurer had a problem deploying to docs-build-and-deploy March 25, 2024 21:22 — with GitHub Actions Failure
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 21:24 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 22:06 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 22:09 — with GitHub Actions Inactive
@asmeurer asmeurer temporarily deployed to docs-build-and-deploy March 25, 2024 22:19 — with GitHub Actions Inactive
@asmeurer asmeurer enabled auto-merge March 27, 2024 21:08
@asmeurer asmeurer merged commit 311d0aa into data-apis:main Mar 27, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofang leofang leofang left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asmeurer @lithomas1 @leofang