JuliaGPU / Adapt.jl

Other
86 stars 24 forks source link

Rename `Adapt.ndims` for clarity #72

Closed vpuri3 closed 6 months ago

vpuri3 commented 8 months ago
julia> Adapt.ndims === Base.ndims
false

If this is intentional, then Adapt.ndims should be renamed to make it clear that it's separate from Base.ndims.

https://github.com/JuliaGPU/Adapt.jl/blob/73ee02288799644e5893ed0eeaab53079d80f938/src/wrappers.jl#L123-L132

Further, I came across this bug which can be resolved by adding a method for LinearAlgebra.Symmetric.

julia> Adapt.ndims(CUDA.ones(4, 4) |> Symmetric |> typeof)
ERROR: UndefVarError: `N` not defined
Stacktrace:
 [1] ndims(::Type{Symmetric{Float32, CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}}})
   @ Adapt ~/.julia/packages/Adapt/yYvku/src/wrappers.jl:132
 [2] top-level scope
   @ REPL[43]:1
 [3] top-level scope
   @ ~/.julia/packages/CUDA/nbRJk/src/initialization.jl:205
maleadt commented 8 months ago

It is intentional; extending Base.ndims would be type piracy. I don't see why it should be renamed; that's just how Julia's namespacing works? It's an internal utility function that has no impact outside of Adapt.jl

vpuri3 commented 8 months ago

Thanks for merging. Yes that's how julia's namespace system works but it can be confusing when one is debugging errors. For example, I spent some time trying to reproduce the error above with Base.ndims until I realized it's actually caused by Adapt.ndims