search

JuliaGPU / Adapt.jl

Other
86 stars 24 forks source link

Incorrect `ndims` reporting due to possible method precedence issue #24

Closed DhairyaLGandhi closed 4 years ago

DhairyaLGandhi commented 4 years ago

On Adapt@1.1

project ``` (Tracker) pkg> st Project Tracker v0.2.6 Status `~/.julia/dev/Tracker/Project.toml` [79e6a3ab] Adapt v1.1.0 [b552c78f] DiffRules v1.0.1 [f6369f11] ForwardDiff v0.10.10 [1914dd2f] MacroTools v0.5.5 [872c559c] NNlib v0.6.6 [77ba4419] NaNMath v0.3.3 [ae029012] Requires v1.0.1 [276daf66] SpecialFunctions v0.8.0 [37e2e46d] LinearAlgebra [de0858da] Printf [9a3f8284] Random [10745b16] Statistics [8dfed614] Test ```

This works as expected:

julia> ndims(LinearAlgebra.Transpose{Float64,Array{Float64,1}})
2

julia> @which ndims(LinearAlgebra.Transpose{Float64,Array{Float64,1}})
ndims(::Type{#s66} where #s66<:AbstractArray{T,N}) where {T, N} in Base at abstractarray.jl:169

On Adapt@2

project ``` (Tracker) pkg> st Project Tracker v0.2.6 Status `~/Downloads/new_clones/Tracker.jl/Project.toml` [79e6a3ab] Adapt v2.0.0 [b552c78f] DiffRules v1.0.1 [f6369f11] ForwardDiff v0.10.10 [1914dd2f] MacroTools v0.5.5 [872c559c] NNlib v0.7.0 [77ba4419] NaNMath v0.3.3 [ae029012] Requires v1.0.1 [276daf66] SpecialFunctions v0.10.3 [37e2e46d] LinearAlgebra [de0858da] Printf [9a3f8284] Random [10745b16] Statistics [8dfed614] Test ```

The WrappedArray version gets called, and returns the incorrect result

julia> ndims(LinearAlgebra.Transpose{Float64,Array{Float64,1}})
1

julia> @which ndims(LinearAlgebra.Transpose{Float64,Array{Float64,1}})
ndims(::Type{#s25} where #s25<:Union{Base.ReinterpretArray{T,N,#s17,A} where A<:AbstractArray{#s17,N} where #s17<:Src, Base.ReshapedArray{T,N,#s16,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where #s16<:Src, SubArray{T,N,#s14,I,L} where L where I where #s14<:Src, Adjoint{T,#s18} where #s18<:Dst, Diagonal{T,#s24} where #s24<:Dst, LowerTriangular{T,#s20} where #s20<:Dst, Transpose{T,#s19} where #s19<:Dst, Tridiagonal{T,#s25} where #s25<:Dst, UnitLowerTriangular{T,#s21} where #s21<:Dst, UnitUpperTriangular{T,#s23} where #s23<:Dst, UpperTriangular{T,#s22} where #s22<:Dst, PermutedDimsArray{T,N,#s13,#s12,#s15} where #s15<:Src where #s12 where #s13}) where {T, N, Src, Dst} in Adapt at /Users/dhairyagandhi/.julia/packages/Adapt/98G0f/src/wrappers.jl:62

Even if it were reporting the right ndims should it count as piracy?

DhairyaLGandhi commented 4 years ago

Simple mwe:

julia> using LinearAlgebra

julia> ndims(LinearAlgebra.Transpose{Float64,Array{Float64,1}})
2

julia> using Adapt

julia> ndims(LinearAlgebra.Transpose{Float64,Array{Float64,1}})
1
maleadt commented 4 years ago

Ah, that's unfortunate. Extending ndims feels like a form of piracy indeed, but on the other hand we need exactly that definition to reconstruct the wrapper so it seems silly to use a different name for that property. It would also be breaking, so let's postpone that for a next major release.