-
Notifications
You must be signed in to change notification settings - Fork 52
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
Address a few device related issues #259
Conversation
Let's merge #171 first. |
@leofang Following up here: are there things related to this PR that we should discuss and resolve during the next consortium meeting? |
PTAL! |
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 Leo. I made some suggestions to try and avoid the average reader to keep wondering what the message here. We are building in an escape hatch here for libraries by allowing arbitrary device and stream objects; the downside of that is that the code then becomes non-portable, and it should be clear that there's no obligation on implementers to provide or support such objects.
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.
Small textual changes, some of which should probably be reviewed to ensure that the original intent is preserved. Some of the textual changes are intended to mirror language used elsewhere in this specification.
Co-authored-by: Athan <kgryte@gmail.com>
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, @kgryte! The suggested changes look good and I've applied them. I left a quick question below.
My only whine at this point is without using a library-specific device object, there is no way to initialize an array on an arbitrary device, nor can I move it to any other device when it's created on the default device. Can someone remind me why we made this decision choice?
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.
LGTM. Any additional changes can be made in follow-up PRs. Thanks, @leofang!
Close #256. Close #156.
This PR addresses a few things:
stream
to.to_device()
and allow any library-specific stream object to be used.to_device()
is to copy (not move) data across devices