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

default to convert #1

Closed
CarloLucibello opened this issue May 18, 2018 · 4 comments
Closed

default to convert #1

CarloLucibello opened this issue May 18, 2018 · 4 comments

Comments

@CarloLucibello
Copy link

CarloLucibello commented May 18, 2018

why we have this

adapt_(T, x) = x

instead of

adapt_(T, x) = convert(T, x)

so that adapt could be used as an opt-out replacement to convert?

Bye
C

@DilumAluthge
Copy link

DilumAluthge commented Jul 19, 2018

@CarloLucibello @MikeInnes Any thoughts on this? I think that convert is the most appropriate fallback.

@MikeInnes
Copy link
Contributor

The problem is that adapt can be called with anything, e.g. adapt(CuArray, 2.0). In cases like that you don't want to do anything, since 2.0 is already GPU compatible (and conversion is nonsensical anyway). At least in the GPU case, there's essentially only one type you want to convert (Array) and everything else should be left alone, so it makes a lot more sense for this to be opt-in.

It might be good to see motivating use cases where opt-out would be better, but the answer might be that you just want a different construct than adapt.

@DilumAluthge
Copy link

DilumAluthge commented Jul 23, 2018

Ahhh good point. What if we did something like this:

function adapt_(T, x)
    try
        convert(T, x)
    catch
        x
    end
end

It’s certainly not the prettiest code, but it has the desired effect - for example, in the CuArrays example, arrays will be converted to CuArrays, and other types will stay the same.

Sent with GitHawk

@maleadt
Copy link
Member

maleadt commented Oct 22, 2024

This is not going to happen at this point anymore, as it would be a significant change to a package that's been stable for a very long time. A try/catch in there would also be expensive, so doesn't seem like a reasonable solution. It would also open the way for nonsensical conversions (e.g., convert(Array, ::Number)) -- it's up to the implementation of adapt_storage to determine whether to opt out of the fallback (e.g., adapt_storage(Array, <:AbstractArray) does fall back to convert).

@maleadt maleadt closed this as not planned Won't fix, can't repro, duplicate, stale Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants