Resolve mul!(::Matrix, ::Diagonal, ::Adjoint) ambiguity #27405

tkf · 2018-06-03T21:54:23Z

It looks like there is an ambiguity problem with mul!(::Matrix, ::Diagonal, ::Adjoint) and mul!(::Matrix, ::Diagonal, ::Transpose) in Julia 0.7-alpha. I made a patch to fix it.

Here is how to reproduce the error in Julia 0.7-alpha:

julia> using LinearAlgebra

julia> A = Diagonal([100, 200, 300])
3×3 Diagonal{Int64,Array{Int64,1}}:
 100    ⋅    ⋅
   ⋅  200    ⋅
   ⋅    ⋅  300

julia> B = reshape(collect(1:9), (3, 3))
3×3 Array{Int64,2}:
 1  4  7
 2  5  8
 3  6  9

julia> Y = zeros(3, 3)
3×3 Array{Float64,2}:
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

julia> mul!(Y, A, B')
ERROR: MethodError: LinearAlgebra.mul!(::Array{Float64,2}, ::Diagonal{Int64,Array{Int64,1}}, ::Adjoint{Int64,Array{Int64,2}}
) is ambiguous. Candidates:
  mul!(out::AbstractArray{T,2} where T, A::Diagonal, in::AbstractArray{T,2} where T) in LinearAlgebra at /buildworker/worker
/package_linux64/build/usr/share/julia/stdlib/v0.7/LinearAlgebra/src/diagonal.jl:271
  mul!(C::AbstractArray{T,2} where T, A::Union{AbstractArray{T,1}, AbstractArray{T,2}} where T, adjB::Adjoint{#s624,#s623} w
here #s623<:(Union{AbstractArray{T,1}, AbstractArray{T,2}} where T) where #s624) in LinearAlgebra at /buildworker/worker/pac
kage_linux64/build/usr/share/julia/stdlib/v0.7/LinearAlgebra/src/matmul.jl:306
Possible fix, define
  mul!(::AbstractArray{T,2} where T, ::Diagonal, ::Adjoint{#s624,#s623} where #s623<:(Union{AbstractArray{T,1}, AbstractArra
y{T,2}} where T) where #s624)

julia> mul!(Y, A, transpose(B)) 
ERROR: MethodError: LinearAlgebra.mul!(::Array{Float64,2}, ::Diagonal{Int64,Array{Int64,1}}, ::Transpose{Int64,Array{Int64,2}}) is ambiguous. Candidates:
  mul!(out::AbstractArray{T,2} where T, A::Diagonal, in::AbstractArray{T,2} where T) in LinearAlgebra at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v0.7/LinearAlgebra/src/diagonal.jl:271 
  mul!(C::AbstractArray{T,2} where T, A::Union{AbstractArray{T,1}, AbstractArray{T,2}} where T, transB::Transpose{#s624,#s623} where #s623<:(Union{AbstractArray{T,1}, AbstractArray{T,2}} where T) where #s624) in LinearAlgebra at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v0.7/LinearAlgebra/src/matmul.jl:251
Possible fix, define 
  mul!(::AbstractArray{T,2} where T, ::Diagonal, ::Transpose{#s624,#s623} where #s623<:(Union{AbstractArray{T,1}, AbstractArray{T,2}} where T) where #s624)

Defining the following method (added in this PR) resolves the ambiguity:

julia> LinearAlgebra.mul!(out::AbstractMatrix, A::Diagonal, 
                          in::Adjoint{T, <: AbstractMatrix{T}}) where T =
           out .= A.diag .* in
       
julia> mul!(Y, A, B')
3×3 Array{Float64,2}:
  100.0   200.0   300.0
  800.0  1000.0  1200.0
 2100.0  2400.0  2700.0

julia> LinearAlgebra.mul!(out::AbstractMatrix, A::Diagonal,
                          in::Transpose{T, <: AbstractMatrix{T}}) where T =
           out .= A.diag .* in
       
julia> mul!(Y, A, transpose(B))
3×3 Array{Float64,2}:
  100.0   200.0   300.0
  800.0  1000.0  1200.0
 2100.0  2400.0  2700.0

julia> versioninfo()
Julia Version 0.7.0-alpha.4
Commit 1538bced17 (2018-05-31 20:57 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Xeon(R) CPU E5-2650 v4 @ 2.20GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.0 (ORCJIT, broadwell)

and mul!(::Matrix, ::Diagonal, ::Transpose) as well.

Sacha0 · 2018-06-03T22:19:32Z

Much thanks for the report and patch @tkf!

This issue points to a deeper issue with CI: The global method ambiguity check should run for every pull request. Hence whichever pull request introduced this ambiguity should have made CI unhappy, and all CI runs since that pull request merged should have failed as well. But clearly that's not the case, so something is amiss.

tkf · 2018-06-03T22:21:06Z

FYI I simplify the method signature a little bit and force-pushed:

@@ -272,8 +272,8 @@ mul!(out::AbstractMatrix, A::Diagonal, in::AbstractMatrix) = out .= A.diag .* in
 mul!(out::AbstractMatrix, A::Adjoint{<:Any,<:Diagonal}, in::AbstractMatrix) = out .= adjoint.(A.parent.diag) .* in
 mul!(out::AbstractMatrix, A::Transpose{<:Any,<:Diagonal}, in::AbstractMatrix) = out .= transpose.(A.parent.diag) .* in
 
-mul!(out::AbstractMatrix, A::Diagonal, in::Adjoint{T, <: AbstractMatrix{T}}) where {T} = out .= A.diag .* in
-mul!(out::AbstractMatrix, A::Diagonal, in::Transpose{T, <: AbstractMatrix{T}}) where {T} = out .= A.diag .* in
+mul!(out::AbstractMatrix, A::Diagonal, in::Adjoint{<:Any,<:AbstractMatrix}) = out .= A.diag .* in
+mul!(out::AbstractMatrix, A::Diagonal, in::Transpose{<:Any,<:AbstractMatrix}) = out .= A.diag .* in
 
 # ambiguities with Symmetric/Hermitian
 # RealHermSymComplex[Sym]/[Herm] only include Number; invariant to [c]transpose

tkf · 2018-06-03T22:22:37Z

@Sacha0 Is it a CI issue? I just thought simply there are not enough test cases to cover it. Or are there magics to generate this kind of ambiguity test cases?

tkf · 2018-06-03T22:27:27Z

I realized that his PR does not fix mul!(Y, Adjoint(A), B') (although it fixes mul!(Y, A', B') since adjoint(A) is not "lazy"). Here is how to produce the error and fix it:

julia> using LinearAlgebra

julia> A = Diagonal([100, 200, 300])
3×3 Diagonal{Int64,Array{Int64,1}}:
 100    ⋅    ⋅
   ⋅  200    ⋅
   ⋅    ⋅  300

julia> B = reshape(collect(1:9), (3, 3))
3×3 Array{Int64,2}:
 1  4  7
 2  5  8
 3  6  9

julia> Y = zeros(3, 3)
3×3 Array{Float64,2}:
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

julia> mul!(Y, Adjoint(A), B')
ERROR: MethodError: LinearAlgebra.mul!(::Array{Float64,2}, ::Adjoint{Int64,Diagonal{Int64,Array{Int64,1}}}, ::Adjoint{Int64,Array{Int64,2}}) is ambiguous. Candidates:
  mul!(out::AbstractArray{T,2} where T, A::Adjoint{#s624,#s623} where #s623<:Diagonal where #s624, in::AbstractArray{T,2} where T) in LinearAlgebra at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v0.7/LinearAlgebra/src/diagonal.jl:272
  mul!(C::AbstractArray{T,2} where T, adjA::Adjoint{#s624,#s623} where #s623<:(Union{AbstractArray{T,1}, AbstractArray{T,2}} where T) where #s624, adjB::Adjoint{#s622,#s621} where #s621<:(Union{AbstractArray{T,1}, AbstractArray{T,2}} where T) where #s622) in LinearAlgebra at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v0.7/LinearAlgebra/src/matmul.jl:316
Possible fix, define
  mul!(::AbstractArray{T,2} where T, ::Adjoint{#s624,#s623} where #s623<:Diagonal where #s624, ::Adjoint{#s622,#s621} where #s621<:(Union{AbstractArray{T,1}, AbstractArray{T,2}} where T) where #s622)

julia> LinearAlgebra.mul!(out::AbstractMatrix,
                          A::Adjoint{<:Any, <:Diagonal},
                          in::Adjoint{<:Any, <:AbstractMatrix}) =
           out .= adjoint.(A.parent.diag) .* in

julia> mul!(Y, Adjoint(A), B')
3×3 Array{Float64,2}:
  100.0   200.0   300.0
  800.0  1000.0  1200.0
 2100.0  2400.0  2700.0

I made a commit and testing it locally now...

and same for Transpose.

Sacha0 · 2018-06-03T22:41:05Z

A little sleuthing revealed that: 1) the method ambiguity check has been running and passing on CI; but 2) the check has not been updated with the birth of new stdlib packages, which now don't get checked; and consequently 3) LinearAlgebra has accumulated a few ambiguities from one or another recent pull request:

julia> using LinearAlgebra

julia> using Test

julia> length(Test.detect_ambiguities(LinearAlgebra; imported=true, recursive=true, ambiguous_bottom=false))
10

julia> versioninfo()
Julia Version 0.7.0-alpha.20
Commit c8ce43ad17* (2018-06-02 06:26 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin17.5.0)
  CPU: Intel(R) Core(TM) i5-7360U CPU @ 2.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.0 (ORCJIT, skylake)

Beyond addressing the method ambiguities now present in LinearAlgebra, we should check for and fix any method ambiguities in other stdlib packages, and set up ambiguity checks for all stdlib packages. I will open a tracking issue shortly. Best!

tkf · 2018-06-03T22:46:11Z

Ah, I didn't know detect_ambiguities. Running it in CI for all stdlibs sounds great. Thanks for the explanation!

andreasnoack · 2018-06-03T22:52:52Z

@JeffBezanson @Keno This is an example of what I had in mind when I asked the ambiguity questions. Both of the methods involved are fine

julia> invoke(mul!, Tuple{AbstractMatrix, Diagonal, AbstractMatrix}, C, D, A')
4×4 Array{Float64,2}:
 -0.249382   1.0107    -0.157625  0.0578353
 -1.12559   -2.38596   -0.621022  0.979386
 -0.253633   0.106486  -0.123275  0.0503601
  1.81893   -1.42479    0.937     0.22742

julia> invoke(mul!, Tuple{AbstractMatrix, AbstractMatrix, Adjoint}, C, D, A')
4×4 Array{Float64,2}:
 -0.249382   1.0107    -0.157625  0.0578353
 -1.12559   -2.38596   -0.621022  0.979386
 -0.253633   0.106486  -0.123275  0.0503601
  1.81893   -1.42479    0.937     0.22742

(although not equally efficient but that was not my argument)

It's a bit annoying that we have to implement all so many redundnat method to disambiguate so I'm wondering if we should restrict mul!(::AbstractMatrix, ::AbstractMatrix, ::Adjoint) and to StridedMatrix and just rely mul!(::AbstractMatrix, ::AbstractMatrix, ::AbstractMatrix) when the matrices aren't subtypes of StridedMatrix.

during tests, to resolve the ambiguity. See: JuliaLang/julia#27405

* Add a test to check method ambiguities * Use StridedMatrix to avoid method ambiguity; fixes #27405 * Avoid method ambiguity in promote_leaf_eltypes This change is only for making Test.detect_ambiguities and shouldn't introduce any change in working code (unless it depends on MethodError). Before this change, we have: julia> LinearAlgebra.promote_leaf_eltypes(()) ERROR: MethodError: LinearAlgebra.promote_leaf_eltypes(::Tuple{}) is ambiguous. Candidates: With this change, we have: julia> LinearAlgebra.promote_leaf_eltypes(()) Bool (cherry picked from commit 4498d27)

* Add a test to check method ambiguities * Use StridedMatrix to avoid method ambiguity; fixes #27405 * Avoid method ambiguity in promote_leaf_eltypes This change is only for making Test.detect_ambiguities and shouldn't introduce any change in working code (unless it depends on MethodError). Before this change, we have: julia> LinearAlgebra.promote_leaf_eltypes(()) ERROR: MethodError: LinearAlgebra.promote_leaf_eltypes(::Tuple{}) is ambiguous. Candidates: With this change, we have: julia> LinearAlgebra.promote_leaf_eltypes(()) Bool

* Add a test to check method ambiguities * Use StridedMatrix to avoid method ambiguity; fixes #27405 * Avoid method ambiguity in promote_leaf_eltypes This change is only for making Test.detect_ambiguities and shouldn't introduce any change in working code (unless it depends on MethodError). Before this change, we have: julia> LinearAlgebra.promote_leaf_eltypes(()) ERROR: MethodError: LinearAlgebra.promote_leaf_eltypes(::Tuple{}) is ambiguous. Candidates: With this change, we have: julia> LinearAlgebra.promote_leaf_eltypes(()) Bool (cherry picked from commit 4498d27)

fredrikekre added the domain:linear algebra Linear algebra label Jun 3, 2018

fredrikekre requested a review from Sacha0 June 3, 2018 22:02

Disambiguate mul!(::Matrix, ::Diagonal, ::Adjoint)

7106c1a

and mul!(::Matrix, ::Diagonal, ::Transpose) as well.

tkf force-pushed the mul-diagonal-adjoint branch from 01b9b55 to 7106c1a Compare June 3, 2018 22:19

Disambiguate mul!(::Matrix, ::Adjoint{<:Any, <:Diagonal}, ::Adjoint)

c9b0de8

and same for Transpose.

Sacha0 mentioned this pull request Jun 3, 2018

method ambiguity checks for stdlib packages #27408

Closed

tkf added a commit to tkf/MatrixCombinators.jl that referenced this pull request Jun 4, 2018

Monkey patch mul!(::Matrix, ::Diagonal, ::Adjoint)

73283c4

during tests, to resolve the ambiguity. See: JuliaLang/julia#27405

tkf added a commit to tkf/MatrixCombinators.jl that referenced this pull request Jun 5, 2018

Monkey patch mul!(::Matrix, ::Diagonal, ::Adjoint)

24a995e

during tests, to resolve the ambiguity. See: JuliaLang/julia#27405

tkf added a commit to tkf/julia that referenced this pull request Aug 19, 2018

Use StridedMatrix to avoid method ambiguity; fixes JuliaLang#27405

a93f1fc

tkf mentioned this pull request Aug 19, 2018

Resolve all method ambiguities in LinearAlgebra #28749

Merged

andreasnoack closed this in 4498d27 Aug 21, 2018

mcognetta mentioned this pull request Aug 24, 2018

[WIP] Optimizing {+,-,*} for structured matrices #28883

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve mul!(::Matrix, ::Diagonal, ::Adjoint) ambiguity #27405

Resolve mul!(::Matrix, ::Diagonal, ::Adjoint) ambiguity #27405

tkf commented Jun 3, 2018

Sacha0 commented Jun 3, 2018

tkf commented Jun 3, 2018

tkf commented Jun 3, 2018 •

edited

tkf commented Jun 3, 2018

Sacha0 commented Jun 3, 2018 •

edited

tkf commented Jun 3, 2018

andreasnoack commented Jun 3, 2018

Resolve mul!(::Matrix, ::Diagonal, ::Adjoint) ambiguity #27405

Resolve mul!(::Matrix, ::Diagonal, ::Adjoint) ambiguity #27405

Conversation

tkf commented Jun 3, 2018

Sacha0 commented Jun 3, 2018

tkf commented Jun 3, 2018

tkf commented Jun 3, 2018 • edited

tkf commented Jun 3, 2018

Sacha0 commented Jun 3, 2018 • edited

tkf commented Jun 3, 2018

andreasnoack commented Jun 3, 2018

tkf commented Jun 3, 2018 •

edited

Sacha0 commented Jun 3, 2018 •

edited