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

to_device should not copy when the operation is a no op #645

Closed
asmeurer opened this issue Jul 5, 2023 · 3 comments · Fixed by #742
Closed

to_device should not copy when the operation is a no op #645

asmeurer opened this issue Jul 5, 2023 · 3 comments · Fixed by #742
Labels
topic: Device Handling Device handling.
Milestone

Comments

@asmeurer
Copy link
Member

asmeurer commented Jul 5, 2023

The spec for to_device() says:

Copy the array from the device on which it currently resides to the specified device.

It doesn't say anything about the case where the given array is already on the specified device.

Note that PyTorch's to has a copy flag, which is False by default, https://pytorch.org/docs/stable/generated/torch.Tensor.to.html, which prevents copying when the device is the same. to_device should probably behave the same. We may or may not want to add a copy flag to to_device as well (but note that you can control this explicitly using asarray).

This came up thinking about this code in scikit-learn https://github.com/scikit-learn/scikit-learn/pull/26315/files/42524bd42900d8ea5f4a334780387a72c6f9580d#diff-86c94a3ca33490c6190f488f5d40b01bf0fd29be36da0b4497ef0da1fda4148a.

@rgommers
Copy link
Member

I think this requires a clarification but not a new keyword. The case of "what if the given device to copy to is the current device" isn't covered in the current text, however I'd expect it to be done with the natural implementation (return self). In the whole design of the standard we do not allow modifying aliased memory, so it's fully equivalent to return self vs. making a data copy in memory. And making a data copy is an execution detail, so it's clear we should never specify such a thing.

@rgommers rgommers added the topic: Device Handling Device handling. label Jul 13, 2023
@kgryte kgryte added this to the v2023 milestone Jul 13, 2023
@leofang
Copy link
Contributor

leofang commented Dec 14, 2023

#626 (comment) discussed the consideration of adding copy to from_dlpack(). Would be nice to apply the same consideration here (to to_device()) too.

@kgryte
Copy link
Contributor

kgryte commented Feb 8, 2024

@leofang Does #741 have any bearing here (e.g., adding a new copy kwarg to to_device)?

E.g., similar to from_dlpack, would you advocate for zero-copy by default when the specified device matches the array's current device? And enforce a copy with a copy kwarg?

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

Successfully merging a pull request may close this issue.

4 participants