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

Move to Array API version 2023.12. #696

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hameerabbasi
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 58.46154% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.36%. Comparing base (cb6b604) to head (a2e84d5).
Report is 101 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #696      +/-   ##
==========================================
- Coverage   90.21%   89.36%   -0.86%     
==========================================
  Files          20       23       +3     
  Lines        3670     3809     +139     
==========================================
+ Hits         3311     3404      +93     
- Misses        359      405      +46     

Copy link
Collaborator

@mtsokol mtsokol left a comment

Choose a reason for hiding this comment

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

I added a few comments. We still need new xfail entries in ci/Finch-array-api-xfails.txt, similarly as you added them to ci/Numba-array-api-xfails.txt.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we want to test these functions. Let's add simple tests, like in NumPy: https://github.com/numpy/numpy/pull/26572/files#diff-db073cec9b943fac08cf9720c471d90dcbb7a0e00f4717433314ec95bee60fe2

return x.astype(dtype, copy=copy)


def unstack(x, /, *, axis=0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think unstack is new - let's add a test for it.

@@ -157,10 +165,16 @@
where,
)
from ._dok import DOK
from ._info import capabilities, default_device, default_dtypes, devices, dtypes
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we shouldn't import them to the main namespace. Let's keep them in __array_namespace_info__ only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current array API tests require these in the main namespace as well.

Copy link
Collaborator

@mtsokol mtsokol Jun 10, 2024

Choose a reason for hiding this comment

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

Hmm then I think it's a bug in the array-api-tests suite.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Array API standard doesn't say they need to be in the main namespace: https://data-apis.org/array-api/latest/API_specification/inspection.html#inspection-apis

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Demonstrated the failure in 42c9bfd, let's wait for array-api-tests fixes as you suggest.

sparse/__init__.py Outdated Show resolved Hide resolved
@hameerabbasi hameerabbasi marked this pull request as draft June 10, 2024 12:44
@hameerabbasi hameerabbasi marked this pull request as draft June 10, 2024 12:44
@hameerabbasi
Copy link
Collaborator Author

hameerabbasi commented Jun 10, 2024

Let's put this back as draft until the different signature for clip is settled. xref data-apis/array-api-tests#262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants