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

MAINT: copy, array-api compatible utility function #19014

Merged
merged 7 commits into from Aug 7, 2023
Merged

Conversation

andyfaff
Copy link
Contributor

@andyfaff andyfaff commented Aug 4, 2023

It seems to me that having an array-api compatible copy function would be quite useful. I have attempted to create one in this PR. Sorry, probably should've sent an item to scipy-dev, but I didn't think it would be controversial.

@lucascolley
Copy link
Member

Agreed that this seems useful, #19005 would make use of this twice but there will probably be other submodules which would use it more.

Copy link
Contributor

@mdhaber mdhaber left a comment

Choose a reason for hiding this comment

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

This would be private, so I don't think an email is necessary. One comment.

@@ -136,3 +136,26 @@ def atleast_nd(x, *, ndim, xp):
x = xp.expand_dims(x, axis=0)
x = atleast_nd(x, ndim=ndim, xp=xp)
return x


def copy(x):
Copy link
Contributor

@mdhaber mdhaber Aug 6, 2023

Choose a reason for hiding this comment

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

Typically copy would not be the only operation that depends on the underlying array library. Do we want every utility function like this separately calling array_namespace to detect the namespace, or should xp be passed in?
(It might be fast enough that it doesn't matter, but I haven't checked.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we probably should pass it in to save effort. I think we should pass it in as a keyword:

def copy(x, *, xp=None):
    if xp is None:
        xp = array_namespace(x)
...

Copy link
Contributor

@mdhaber mdhaber left a comment

Choose a reason for hiding this comment

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

It's arguable that we don't need a one-line wrapper of as_xparray, but I think it makes the intent ("copy this thing", not "make this an array") more clear, so it seems fine.

I checked out the other array_api_compatible tests and this looks consistent with those, so presumably it is actually being tested with a non-regular-NumPy backend (maybe numpy.array_api) on CI.

scipy/_lib/_array_api.py Outdated Show resolved Hide resolved
@mdhaber mdhaber merged commit ef73423 into scipy:main Aug 7, 2023
1 check passed
@andyfaff andyfaff deleted the copy branch August 7, 2023 00:46
@andyfaff
Copy link
Contributor Author

andyfaff commented Aug 7, 2023

I checked out the other array_api_compatible tests and this looks consistent with those, so presumably it is actually being tested with a non-regular-NumPy backend (maybe numpy.array_api) on CI.

That is correct. You can see from these failed CI tests that various backends are used. The CI configuration indicates that pytorch, numpy, numpy.array_api are examined.

@j-bowhay j-bowhay modified the milestones: 1.11.2, 1.12.0 Aug 7, 2023
lucascolley added a commit to lucascolley/scipy that referenced this pull request Aug 10, 2023
[skip cirrus] [skip circle]
lucascolley added a commit to lucascolley/scipy that referenced this pull request Aug 12, 2023
andyfaff added a commit that referenced this pull request Aug 17, 2023
MAINT: use `copy` utility from #19014 in `cluster`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants