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.

Sign up for GitHub

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

Jump to bottom

add device kwarg to astype API #665

Merged
merged 7 commits into from
Sep 19, 2023
Merged

add device kwarg to astype API #665

merged 7 commits into from
Sep 19, 2023

Conversation

steff456
Copy link
Member

This PR,

  • Adds device kwarg to astype API in the DRAFT version

It fixes #647 (comment)

@steff456 steff456 added the API change Changes to existing functions or objects in the API. label Jul 26, 2023
@steff456 steff456 requested review from rgommers and kgryte July 26, 2023 18:21
@steff456 steff456 self-assigned this Jul 26, 2023
@steff456 @kgryte
Add review changes
caca863 
Co-authored-by: Athan <kgryte@gmail.com>
kgryte
kgryte approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM.

@kgryte kgryte added the topic: Device Handling Device handling. label Jul 27, 2023
@kgryte kgryte added this to the v2023 milestone Jul 27, 2023
@rgommers rgommers mentioned this pull request Jul 28, 2023
Closed
leofang
leofang approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

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

LGTM too. One possible nit: IIRC we discussed about raising exceptions for casting to unsupported dtypes by a certain device. Do we want to address it in this PR, or another?

I'd like to have @oleksandr-pavlyk to take a look if possible.

@kgryte
Copy link
Contributor

kgryte commented Jul 31, 2023

@leofang Ah, you're right. We do need to address the condition where the requested device does not support the desired dtype. Although, this applies more generally to all creation functions supporting the dtype kwarg. In which case, maybe we do want to address in a follow-up PR which covers all affected APIs?

@leofang
Copy link
Contributor

leofang commented Jul 31, 2023

Yup sounds good

@ogrisel
Copy link

ogrisel commented Aug 23, 2023

Thanks for the PR, LGTM as well.

kgryte
kgryte approved these changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @steff456!

@kgryte
Copy link
Contributor

kgryte commented Sep 19, 2023

As this PR has three approvals, will go ahead and merge.

We'll submit a follow-up PR to add guidance concerning what should happen when a device does not support an array data type when using astype and various creation functions.

@kgryte kgryte merged commit d39fc66 into data-apis:main Sep 19, 2023
1 check passed
@kgryte kgryte deleted the astype_device branch September 19, 2023 22:29
@ndgrigorian ndgrigorian mentioned this pull request Sep 22, 2023
Closed
@Micky774 Micky774 mentioned this pull request Mar 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofang leofang leofang approved these changes

@kgryte kgryte kgryte approved these changes

@rgommers rgommers Awaiting requested review from rgommers

@oleksandr-pavlyk oleksandr-pavlyk Awaiting requested review from oleksandr-pavlyk

Assignees

@steff456 steff456

Labels
API change Changes to existing functions or objects in the API. topic: Device Handling Device handling.
Projects
None yet
Milestone
v2023
Development

Successfully merging this pull request may close these issues.

Add dtype keyword to to_device
4 participants
@steff456 @kgryte @leofang @ogrisel