JuliaLang / LinearAlgebra.jl

Julia Linear Algebra standard library
Other
14 stars 0 forks source link

`map` over `Adjoint` assumes that the result supports `adjoint` #1015

Open jishnub opened 1 year ago

jishnub commented 1 year ago

The following is broken as a consequence

julia> a = [[1,2,3]]'
1×1 adjoint(::Vector{Vector{Int64}}) with eltype LinearAlgebra.Adjoint{Int64, Vector{Int64}}:
 [1 2 3]

julia> map(size, a)
ERROR: MethodError: no method matching adjoint(::Tuple{Int64, Int64})

Closest candidates are:
  adjoint(::Missing)
   @ Base missing.jl:101
  adjoint(::LinearAlgebra.Givens)
   @ LinearAlgebra ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/givens.jl:60
  adjoint(::LinearAlgebra.UnitLowerTriangular)
   @ LinearAlgebra ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/triangular.jl:409
  ...

Stacktrace:
 [1] (::LinearAlgebra.var"#1#2"{typeof(size)})(xs::Vector{Int64})
   @ LinearAlgebra ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/adjtrans.jl:403
 [2] iterate(::Base.Generator{Vector{Vector{Int64}}, LinearAlgebra.var"#1#2"{typeof(size)}})
   @ Base ./generator.jl:47 [inlined]
 [3] _collect(c::Vector{Vector{Int64}}, itr::Base.Generator{Vector{Vector{Int64}}, LinearAlgebra.var"#1#2"{typeof(size)}}, ::Base.EltypeUnknown, isz::Base.HasShape{1})
   @ Base ./array.jl:852 [inlined]
 [4] collect_similar(cont::Vector{Vector{Int64}}, itr::Base.Generator{Vector{Vector{Int64}}, LinearAlgebra.var"#1#2"{typeof(size)}})
   @ Base ./array.jl:761
 [5] map(f::Function, A::Vector{Vector{Int64}})
   @ Base ./abstractarray.jl:3273
 [6] map(f::Function, avs::LinearAlgebra.Adjoint{LinearAlgebra.Adjoint{Int64, Vector{Int64}}, Vector{Vector{Int64}}})
   @ LinearAlgebra ~/packages/julias/julia-latest/share/julia/stdlib/v1.11/LinearAlgebra/src/adjtrans.jl:403
 [7] top-level scope
   @ REPL[2]:1

julia> VERSION
v"1.11.0-DEV.78"

The broadcasted operation works as expected:

julia> size.(a)
1×1 Matrix{Tuple{Int64, Int64}}:
 (1, 3)

This error happens on v1.9 and 1.10 as well.

martinholters commented 1 year ago

Yeah, that's on purpose (well, not the error, but the intended behavior): https://github.com/JuliaLang/julia/blob/c5e462137298a6876ca8bd0939f6e5ff79996d14/stdlib/LinearAlgebra/src/adjtrans.jl#L404-L410 Indeed, if the element type supports adjoint (or transpose), this seems useful, as it preserves the container being a row-vector instead of a (single-row) matrix, which are subtly different. But we make reasonably make this behavior dependent on whether the (output) element type supports adjoint (transpose) or not, and doing this unconditionally might not have been the best choice. I wonder how severe the breakage is if we unconditionally do NOT do this, i.e. always return a single-row matrix instead... Notably, the comment mentions broadcast which also gives a single-row matrix.

martinholters commented 1 year ago

Notably, the comment mentions broadcast which also gives a single-row matrix.

Not only that, there is also code that's supposed to affect broadcast: https://github.com/JuliaLang/julia/blob/c5e462137298a6876ca8bd0939f6e5ff79996d14/stdlib/LinearAlgebra/src/adjtrans.jl#L413-L417 But apparently, that is ineffective:

julia> sqrt.([4.0; 9.0]')
1×2 Matrix{Float64}:
 2.0  3.0

julia> map(sqrt, [4.0; 9.0]')
1×2 adjoint(::Vector{Float64}) with eltype Float64:
 2.0  3.0

EDIT: ok, it works when explicitly calling broadcast:

julia> broadcast(sqrt, [4.0; 9.0]')
1×2 adjoint(::Vector{Float64}) with eltype Float64:
 2.0  3.0