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

Rename Adapt.ndims for clarity #72

Closed
vpuri3 opened this issue Oct 30, 2023 · 2 comments · Fixed by #75
Closed

Rename Adapt.ndims for clarity #72

vpuri3 opened this issue Oct 30, 2023 · 2 comments · Fixed by #75

Comments

@vpuri3
Copy link
Contributor

vpuri3 commented Oct 30, 2023

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.

Adapt.jl/src/wrappers.jl

Lines 123 to 132 in 73ee022

ndims(::Type{<:Base.LogicalIndex}) = 1
ndims(::Type{<:LinearAlgebra.Adjoint}) = 2
ndims(::Type{<:LinearAlgebra.Transpose}) = 2
ndims(::Type{<:LinearAlgebra.LowerTriangular}) = 2
ndims(::Type{<:LinearAlgebra.UnitLowerTriangular}) = 2
ndims(::Type{<:LinearAlgebra.UpperTriangular}) = 2
ndims(::Type{<:LinearAlgebra.UnitUpperTriangular}) = 2
ndims(::Type{<:LinearAlgebra.Diagonal}) = 2
ndims(::Type{<:LinearAlgebra.Tridiagonal}) = 2
ndims(::Type{<:WrappedArray{<:Any,N}}) where {N} = N

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
Copy link
Member

maleadt commented Oct 31, 2023

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
Copy link
Contributor Author

vpuri3 commented Nov 1, 2023

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

@maleadt maleadt changed the title Adapt.ndims doesn't overload Base.ndims Rename Adapt.ndims for clarity Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants