Skip to content
This repository has been archived by the owner on Mar 12, 2021. It is now read-only.

add implicit float conversion to math functions #671

Closed
CarloLucibello opened this issue Apr 3, 2020 · 4 comments
Closed

add implicit float conversion to math functions #671

CarloLucibello opened this issue Apr 3, 2020 · 4 comments

Comments

@CarloLucibello
Copy link

As mentioned by @maleadt in FluxML/Zygote.jl#574:
"""
Oh right, that was one breaking change in the CuArrays release: Integers used to be eagerly converted by cu to Float32. That's more than questionable, so we got rid of it. However, many NVIDIA intrinsics are not available for integers, so that broke some tests.
"""
I wonder what is the rationale of the conversion removal and if we could re-introduce the old behavior, which is also what Base does, as it may be convenient in some situations.

@maleadt
Copy link
Member

maleadt commented Apr 3, 2020

which is also what Base does

Wait, what? No, Base doesn't do any eager conversions. Having cu([1]) == [1f0] seems like a recipe for disaster, why do you want that conversion back? The fact that you were silently dispatching to floating-point operations when asking for (unsupported) integer versions is an argument for the current behavior, to me.

@CarloLucibello
Copy link
Author

sorry I meant Base converts when doing mathematical operations such as log.([1, 1]), so I would expect the same for log.(cu([1, 1]))

@CarloLucibello
Copy link
Author

I see now that on CuArrays 1.7 cu performed the casting, I agree that should be avoided and current behavior for cu is preferable. On the other hand, I'd like all mathematical operations to work on integer cuarrays through conversion

@CarloLucibello CarloLucibello changed the title restore implicit float conversion add implicit float conversion to math functions Apr 3, 2020
@maleadt
Copy link
Member

maleadt commented Apr 3, 2020

I'd like all mathematical operations to work on integer cuarrays through conversion

Yeah, understandably. We're in the annoying situation now where we've been hoping to reuse Base's conversions through Cassette, only redefining the bottommost calls to incompatible CPU library intrinsics with NVIDIA ones, but progress on Cassette inferability has been... slow. In the meantime, we don't have a good policy or place to add higher-level functions like log accepting integers. I guess we should just add them to CUDAnative for now, e.g. like JuliaGPU/CUDAnative.jl#445. If you have the need, and the time to create a PR, feel free to add some implementations there!

@maleadt maleadt closed this as completed Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants