Skip to content

MAINT: A few updates to the array_api #20066

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
Merged
merged 23 commits into from
Nov 12, 2021
Merged

MAINT: A few updates to the array_api #20066

merged 23 commits into from
Nov 12, 2021

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Oct 7, 2021

A few minor updates to the array_api namespace based on recent updates to the spec.

CC @seberg you may actually want to review this one before it is merged this time.

@asmeurer
Allow casting in the array API asarray()
48005c1
@asmeurer
Restrict multidimensional indexing in the array API namespace
9af4814 
The spec has recently been updated to only require multiaxis (i.e., tuple)
indices in the case where every axis is indexed, meaning there are either as
many indices as axes or the index has an ellipsis.
@seberg
Copy link
Member

seberg commented Oct 7, 2021

The main comment I remember was that you had a lot of complexity to support arr[boolean_arr,] when maybe all you need to support is arr[boolean_arr]. But I guess there was no change/decision w.r.t. to that one made. And isn't the most important point the non-multi-dim index part? arr3d[0] not being strictly defined or did you decide against that after all?

mattip
mattip reviewed Oct 10, 2021
View reviewed changes
Copy link
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

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

linting

@asmeurer
Fix type promotion for numpy.array_api.where
cb0b9c6 
where does value-based promotion for 0-dimensional arrays, so we use the same
trick as in the Array operators to avoid this.
@asmeurer
Copy link
Member Author

I pushed a fix to the type promotion on where. As far as I can tell, the fix suggested at #18585 (comment) wouldn't work since where isn't a ufunc.

@asmeurer
Print empty array_api arrays using empty()
7da7a99 
Printing behavior isn't required by the spec. This is just to make things
easier to understand, especially with the array API test suite.
@asmeurer
Fix an incorrect slice bounds guard in the array API
0169af7
asmeurer added a commit to data-apis/array-api-tests that referenced this pull request Oct 27, 2021
@asmeurer
Add test that svd values are sorted
139e83e 
This test will not pass NumPy without the changes in
numpy/numpy#20066 due to an update in indexing
behavior in the spec.
@asmeurer
Disallow multiple different dtypes in the input to np.array_api.meshgrid
e0589fd
@asmeurer
Remove DLPack support from numpy.array_api.asarray()
21faf34 
from_dlpack() should be used to create arrays using DLPack.
@asmeurer
Remove __len__ from the array API array object
d0f591d
@asmeurer
Add astype() to numpy.array_api
17d7886
@asmeurer
Update the unique_* functions in numpy.array_api
7d9edf3 
unique() in the array API was replaced with three separate functions,
unique_all(), unique_inverse(), and unique_values(), in order to avoid
polymorphic return types.

Additionally, it should be noted that these functions to not currently conform
to the spec with respect to NaN behavior. The spec requires multiple NaNs to
be returned, but np.unique() returns a single NaN. Since this is currently an
open issue in NumPy to possibly revert, I have not yet worked around this. See
numpy#20326.
@asmeurer
Add the stream argument to the array API to_device method
cb335d2 
This does nothing in NumPy, and is just present so that the signature is valid
according to the spec.
@asmeurer
Use the NamedTuple classes for the type signatures
5cae94d
@asmeurer
Add unique_counts to the array API namespace
f6053aa
@asmeurer
Remove some unused imports
680e0a4
@asmeurer
Update the array_api indexing restrictions
49a3cc9 
The "multiaxis indexing must index every axis explicitly or use an ellipsis"
was supposed to include any type of index, not just tuple indices.
@asmeurer
Merge branch 'main' into array-api-updates2
475b01d
@asmeurer
Copy link
Member Author

asmeurer commented Nov 9, 2021

This is ready for review. There have been several small changes to the spec since the last time anyone has looked at this. With this PR, everything should now be up-to-date, except for a handful of things which are being updated in separate pull requests.

@asmeurer asmeurer mentioned this pull request Nov 9, 2021
Closed
@rgommers
Copy link
Member

@asmeurer there is one test failure, test_device_property. Could you fix that?

@rgommers
Copy link
Member

The boolean indexing question higher up (#20066 (comment)) also deserves a reply.

rgommers
rgommers approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This all LGTM modulo the test failure.

@asmeurer
Copy link
Member Author

@asmeurer there is one test failure, test_device_property. Could you fix that?

Sorry about that. #20335 has messed up my ability to run the tests locally.

@asmeurer
Copy link
Member Author

The main comment I remember was that you had a lot of complexity to support arr[boolean_arr,] when maybe all you need to support is arr[boolean_arr]. But I guess there was no change/decision w.r.t. to that one made. And isn't the most important point the non-multi-dim index part? arr3d[0] not being strictly defined or did you decide against that after all?

I did address the point of treating a[0] and a[0,] differently. That was just a misreading of the spec on my part. I think we could drop a[boolean_arr,] if that simplifies the code. What was the suggested way to do that? Any refactoring of the index validation should be fine so long as it passes the tests in test_array_object.py as well as the test_indexing test in the array API testsuite.

asmeurer and others added 6 commits November 10, 2021 17:29
@asmeurer @BvB93
Use a simpler type annotation for the array API to_device method
6352d11 
Co-authored-by: Bas van Beek <43369155+BvB93@users.noreply.github.com>
@asmeurer
Fix a typo
b7bf06e
@asmeurer
Fix a test failure in the array_api submodule
61bc679 
The array_api cannot use the NumPy testing functions because array_api arrays
do not mix with NumPy arrays, and also NumPy testing functions may use APIs
that aren't supported in the array API.
@asmeurer
Merge branch 'array-api-updates2' of github.com:asmeurer/numpy into a…
f72ed85 
…rray-api-updates2
@asmeurer
Merge branch 'main' into array-api-updates2
09aa2b5
@asmeurer
Add dlpack support to the array_api submodule
580a616
@rgommers rgommers added this to the 1.22.0 release milestone Nov 12, 2021
@rgommers
Copy link
Member

This all looks good. CI failures are okay: linter ones are uninteresting, the typing error was fixed in main yesterday, and the segfault is unrelated (happening in other PRs). So in this goes.

I did address the point of treating a[0] and a[0,] differently. That was just a misreading of the spec on my part. I think we could drop a[boolean_arr,] if that simplifies the code. What was the suggested way to do that?

Seems fine either way to me, and can be done in a separate follow-up given that it's a discussion that predates this PR. Let's see if @seberg has a preference here.

@rgommers
Copy link
Member

Ah wait, there's even more failures. They all look unrelated, but rerunning CI anyway to get this a bit more green.

@rgommers
Copy link
Member

Looking much better now, in it goes. Thanks @asmeurer & reviewers!

@rgommers rgommers merged commit ff2e2a1 into numpy:main Nov 12, 2021
@seberg
Copy link
Member

seberg commented Nov 12, 2021

A followup is fine and would be nice. I don't recall the logic exactly, what I do remember is that the indexing check was very complex (with some recursive calls), and a lot of that complexity was there to accomodate arr[boolean_arr,]. I believe in the original PR I had proposed a simplification.
That said, I think it is correct, just seemed more complex than necessary.

@seberg
Copy link
Member

seberg commented Nov 12, 2021

PS: And since this is experimental and a corner case, I don't think it would matter if you change arr[boolean_arr,] from being allowed to being rejected.

leofang pushed a commit to leofang/cupy that referenced this pull request Nov 12, 2021
@asmeurer @leofang
apply diff from numpy/numpy#20066
797b2af
@leofang leofang mentioned this pull request Nov 12, 2021
Merged
cr313 added a commit to cr313/test-array-api that referenced this pull request Apr 19, 2024
@cr313
Add test that svd values are sorted
43ff81d 
This test will not pass NumPy without the changes in
numpy/numpy#20066 due to an update in indexing
behavior in the spec.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattip mattip mattip left review comments

@leofang leofang leofang left review comments

@BvB93 BvB93 BvB93 left review comments

@rgommers rgommers rgommers approved these changes

Assignees
No one assigned
Labels
03 - Maintenance component: numpy.array_api
Projects
None yet
Milestone
1.22.0 release
Development

Successfully merging this pull request may close these issues.
6 participants
@asmeurer @seberg @rgommers @mattip @leofang @BvB93