Skip to content

array_api_compat.numpy.asarray(torch.Tensor) returns a torch.Tensor #106

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

Closed
izaid opened this issue Mar 10, 2024 · 15 comments
Closed

array_api_compat.numpy.asarray(torch.Tensor) returns a torch.Tensor #106

izaid opened this issue Mar 10, 2024 · 15 comments

Comments

@izaid
Copy link

izaid commented Mar 10, 2024

This feels like a bug to me. My expectation is that xp.asarray converts to an array object in the xp namespace if that operation is supported.

>>> import torch
>>> import array_api_compat.numpy
>>> x = torch.arange(10)
>>> array_api_compat.numpy.asarray(x)
tensor([0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
@izaid izaid changed the title array_api_compat.numpy.asarray(torch.Tensor) returns a `torch.Tensor array_api_compat.numpy.asarray(torch.Tensor) returns a torch.Tensor Mar 10, 2024
@rgommers
Copy link
Member

Agreed, that is a bug. This line looks wrong:

@ivirshup
Copy link
Contributor

ivirshup commented Mar 11, 2024

FWIW, this bug seems to have been introduced in 1.5, and had the correct behaviour in 1.4.

Also is not limited to this case, for instance I ran into this for array_api_compat.cupy.asarray(np.ndarray)

@ivirshup
Copy link
Contributor

What is the expected behavior for xp.asarray(some_other_array_type)? Are there cases it should fail in?

@asmeurer
Copy link
Member

If the input type is one of the types mentioned in the standard (in the type signature), it should convert that input type. Otherwise, generally for array-api-compat, any behavior outside of the standard should just pass through to the library function. So in this case, asarray() should indeed convert the input because normal np.asarray(Tensor) works (and also I think Tensor falls under the category of SupportsBufferProtocol).

@asmeurer
Copy link
Member

asmeurer commented Mar 11, 2024

Bisected it to this commit, which made that line apply to objects with __array__, instead of just objects that are already ndarray subclasses. 8ee1613 CC @lithomas1

@lithomas1
Copy link
Contributor

lithomas1 commented Mar 11, 2024

Yeah, I messed up asarray. Sorry about that. I have a WIP branch fixing that ,and some other dask stuff.

@asmeurer
Copy link
Member

OK, I'll hold off on looking at this for now so I don't step on your toes. You may want to integrate the test from https://github.com/data-apis/array-api-compat/pull/109/files. It may also be necessary to update the copy logic generally similar to data-apis/array-api-strict#15. I don't know if the "check if a copy was made" trick can work for cupy and dask, but if it can we should use it. If there end up being too many library-specific checks in the asarray code it may be more prudent to just split it into separate helpers for each library.

@lithomas1
Copy link
Contributor

@izaid @ivirshup

Are you able to test main again? I should have this bug fixed, but just wanted to make sure.

@izaid
Copy link
Author

izaid commented Mar 19, 2024

@izaid @ivirshup

Are you able to test main again? I should have this bug fixed, but just wanted to make sure.

Is there a link somewhere with steps to follow? I only have array_api_compat installed from pip.

@lucascolley
Copy link
Member

lucascolley commented Mar 19, 2024

  • clone array_api_compat main
  • cd array_api_compat
  • create a new env
  • pip install torch
  • python
  • then run your example above

should work?

@ivirshup
Copy link
Contributor

Looks like 94e42df works for cupy for me. Thanks!

Unfortunately, did have to nuke my cuda install, but I'm pretty confident that's unrelated.

@jowodo
Copy link

jowodo commented Mar 20, 2024

will this fix be released in version 1.5.1 and if so when approximately?
thanks :)

@asmeurer
Copy link
Member

I'm hoping to get a release out before the end of the week. I want to fix a few other issues that have cropped up too.

@asmeurer
Copy link
Member

I just released 1.5.1 with this fix (and some others).

@asmeurer
Copy link
Member

By the way, this still won't work for something like np.asarray(cupy.array(...)) because CuPy explicitly errors when there would be an implicit device transfer to CPU.

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

No branches or pull requests

7 participants