JuliaGPU / Adapt.jl

Other

86 stars 24 forks source link

Make ndims and parent inferrable #33

Closed martenlienen closed 3 years ago

martenlienen commented 3 years ago

Previously, the @isdefined conditionals prevented julia from inferring the type of these methods at compile time. With this change broadcast operations in CUDA.jl like C .- a' can now be fully inferred which was previously impossible because the BroadcastStyle (which uses ndims internally) could not be decided at compile time.

maleadt commented 3 years ago

Thanks. I guess this doesn't hurt, but it reminds me that we really shouldn't be using these methods here: https://github.com/JuliaGPU/Adapt.jl/issues/30

maleadt commented 3 years ago

Maybe now's a good time as ever to bite that bullet; with the 1.6 upgrade requiring breaking upgrades to most downstream packages anyway.

martenlienen commented 3 years ago

I think I don't understand the whole context. Does that mean that this PR is obsolete? Or are you proposing a different change? Anyway it is nice that Julia can now type infer broadcasted CUDA expressions, at least in my local dev version.

maleadt commented 3 years ago

No, this is still useful. Let me first fix CI though.

codecov[bot] commented 3 years ago

Codecov Report

Merging #33 (1b99b81) into master (1edd29c) will increase coverage by 5.71%. The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   62.85%   68.57%   +5.71%     
==========================================
  Files           3        3              
  Lines          35       35              
==========================================
+ Hits           22       24       +2     
+ Misses         13       11       -2

Impacted Files	Coverage Δ
src/wrappers.jl	`70.37% <33.33%> (+7.40%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1edd29c...1b99b81. Read the comment docs.

marius311 commented 3 years ago

Just went down a rabbit hole of why my view(::CuArray) broadcasts weren't inferring, which ultimately led me to this PR, which fixes it. Much appreciated if this could be tagged in the near future so I can add it as a compat bound.

maleadt commented 3 years ago

https://github.com/JuliaRegistries/General/pull/27297