-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add device
kwarg support to can_cast
and result_type
#691
Conversation
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'm not sure I like to add the device=
keyword here. As discussed in gh-672, one can already make this work by passing in an array. So why not add a note instead that in case one worries about device-specific behavior, use an array on the device rather than a dtype as the first input? That avoids the need to add the keyword to these APIs in all array libraries, for most of which this will be a no-op. And it's anyway a better idea imho, because the only way to obtain a device instance is from an array, so can_cast(x, xp.float64)
should be preferred over can_cast(xp.float32, xp.float32, device=x.device)
. In case you want this kind of logic without having an array at hand, use can_cast(xp.empty([], dtype=..., device=...), xp.float64)
.
@rgommers Consider For IMO, by default, these APIs should return consistent values--namely, by strictly applying type promotion rules as defined in the specification--for the sake of predictability. The |
Also, adding a note here would imply that returning device-specific return values was somehow the expected behavior in prior versions of the spec. I don't think that is true given the existing language.
If we update the spec to infer the device from input arrays and then allow returning results only in accordance with that device, I'd consider that a breaking change to the spec. |
It can raise or be undefined behavior to have multiple arrays on different devices. We don't allow combining arrays that aren't on the same device in any other API either.
You can use
I don't quite agree. It is ill-specified now, it says nothing of relevance either way in case a dtype is missing on a specific device. We didn't consider that case at all until recently. It is and remains a bit of a corner case, so we're clarifying the expectation here. It doesn't seem reasonable to me to take a lack of precision in previous releases for a corner case to extrapolate that we must add non-useful keywords to libraries like numpy. |
@oleksandr-pavlyk Did you have further thoughts you wanted to share either here or on #672? |
The type promotion rules result in a graph where dtypes are the nodes. Functions Some data types may be unsupported on certain devices, changing the graph. The What remains to clarify is how to aggregate
I think the spec should also provide some rules about type promotion graphs for a proper subset of dtypes. |
Those rules seem reasonable to me. |
@rgommers To be clear, you originally raised objections to adding |
Actually, re-reading the whole discussion, I think no one answered my comments around this already being possible without adding device keywords to these functions. @oleksandr-pavlyk wrote on the linked issue:
There is no way to get a device object in a library-independent way other than taking it from an array. So this use case may not be relevant, and if it is then I'd say that it's as easy to do:
or
as it is to do
Hence, unless I am missing something, this is still not compelling and already supported - we should reject this change I believe. |
As discussed in #672 (comment), current consensus is to not add a However, we will add, in a separate PR, some clarification to the text regarding how the respective functions should behave when provided input arrays, rather than dtypes, where the array device should be taken into consideration and reflect the device-specific type promotion graph. Closing this PR... |
This PR
resolves Device-specific type promotion rules #672 by adding
device
keyword argument support tocan_cast
andresult_type
.adds guidance indicating that, by default, device capabilities should not be considered when applying type promotion rules. Currently, the specification is mum on whether device capabilities should be considered. For both functions, when
device
isNone
, only Array API type promotion rules may be applied. Whendevice
is a device object, consideration can be made as to whether particular type promotion rules are possible. E.g., if a device only supportsfloat32
, thencan return
false
. Otherwise, users have to workaround in which they applycan_cast
and then use the proposed inspection APIs (Add Array API inspection utilities #689) to determine whether a device supports the promoted type. This PR makes this operation more direct.