-
Notifications
You must be signed in to change notification settings - Fork 52
Add API for device support #96
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
fb75adf
Complete the "device support" section
rgommers a90ea43
Add device keywords to the creation functions
rgommers 4b4a250
Add the device object and a device array attribute
rgommers 48fbee9
Update for review comments, narrow scope of syntax and semantics
rgommers ba5a1e7
Remove device object from API specification
rgommers 1ea89be
Address review comments about control method priority, and `*_like`
rgommers 81b1ee8
A small tweak on wording for `.device`
rgommers File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me of an interest inquiry we had a while ago: Should the
_like*()
functions honor the device where the input arrayx
is on? (cupy/cupy#3457)Now I look back, it seems also plausible if the output array is on the same device as
x
, but my rejection still holds: it is incompatible with any sane device management approaches (context manager, explicit function call (such asuse_device(N)
), etc). I suppose having the newly added argumentdevice
will encounter the same challenge. The most radical example isx
on device 1, the argumentdevice
is set to 2, but the default/current device is 0.Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the argument you made on that issue.
I don't see the conflict here. If a library has multiple ways of controlling device placement, the most explicit method should have the highest priority. So:
device=
keyword is specified, that always takes precedencedevice=None
, then use the setting from a context manager, if set.Your example seems very similar to the first example of https://pytorch.org/docs/stable/notes/cuda.html#cuda-semantics, which I think explains desired behaviour here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
empty_like
description seems clear enough: the_like
is aboutshape
anddtype
only.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @rgommers I fully agree with everything you said above, but I noticed in the "Semantics" section that you're adding in this PR, the wording is a bit different and (arguably) less clear. Should we incorporate your above reply there? And it'd be nice to add a variant of the "
_like
is aboutshape
anddtype
only" emphasis to all the docstrings wheredevice
is an optional argument.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good ideas, done.