-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Resolve all method ambiguities in LinearAlgebra #28749
Conversation
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
promote_leaf_eltypes(x::Union{AbstractArray{T},Tuple{Vararg{T}}}) where {T<:Number} = T | ||
promote_leaf_eltypes(x::Union{AbstractArray{T},Tuple{Vararg{T}}}) where {T<:NumberArray} = eltype(T) | ||
promote_leaf_eltypes(x::Union{AbstractArray{T},Tuple{T,Vararg{T}}}) where {T<:Number} = T | ||
promote_leaf_eltypes(x::Union{AbstractArray{T},Tuple{T,Vararg{T}}}) where {T<:NumberArray} = eltype(T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Copied from my commit message] This change is only for supressing 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, considering that we have mapreduce(promote_leaf_eltypes, promote_type, x; init=Bool)
below, I suppose it was intended to return Bool
?
Nice! That is a surprisingly and happily small number of tweaks required. |
* 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)
Thanks @tkf! :) |
Oh no. I think the test I added broke the CI. Some tests did not pass: 597 passed, 1 failed, 0 errored, 0 broken.LinearAlgebra/matmul: Test Failed at /tmp/julia/share/julia/stdlib/v1.1/LinearAlgebra/test/matmul.jl:449
Expression: detect_ambiguities(LinearAlgebra, Base; imported=true, recursive=true) == []
Evaluated: Tuple{Method,Method}[(isequal(x, ::Main.Test80Main_arrayops.totally_not_five26034) in Main.Test80Main_arrayops at /tmp/julia/share/julia/test/arrayops.jl:2459, isequal(::Missing, ::Any) in Base at missing.jl:60), (==(x, ::Main.Test80Main_arrayops.totally_not_five26034) in Main.Test80Main_arrayops at /tmp/julia/share/julia/test/arrayops.jl:2464, ==(w::WeakRef, v) in Base at gcutils.jl:4), (==(::Main.Test80Main_arrayops.totally_not_five26034, x) in Main.Test80Main_arrayops at /tmp/julia/share/julia/test/arrayops.jl:2463, ==(::Any, ::Missing) in Base at missing.jl:55), (==(x, ::Main.Test80Main_arrayops.totally_not_five26034) in Main.Test80Main_arrayops at /tmp/julia/share/julia/test/arrayops.jl:2464, ==(::Missing, ::Any) in Base at missing.jl:54), (==(::Main.Test80Main_arrayops.totally_not_five26034, x) in Main.Test80Main_arrayops at /tmp/julia/share/julia/test/arrayops.jl:2463, ==(w, v::WeakRef) in Base at gcutils.jl:5), (isequal(::Main.Test80Main_arrayops.totally_not_five26034, x) in Main.Test80Main_arrayops at /tmp/julia/share/julia/test/arrayops.jl:2458, isequal(::Any, ::Missing) in Base at missing.jl:61)] == Any[] --- https://travis-ci.org/JuliaLang/julia/jobs/418693949#L2141-L2142 Does |
I noticed that the issue was already filed #28804 |
* 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)
* 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 (cherry picked from commit 4498d27)
This PR has three commits to:
Add a test using
detect_ambiguities
to avoid further introduction of method ambiguities withinLinearAlgebra
. I'm suggesting a similar test across all standard libraries in Check ambiguous methods in stdlib #28665. But I think in general it's nice to have it per-package basis especially if it were to split into a separate repository at some point.Simply apply @andreasnoack's suggestion Resolve mul!(::Matrix, ::Diagonal, ::Adjoint) ambiguity #27405 (comment) and define
mul!(::AbstractMatrix, ::Diagonal, ::StridedMatrix)
instead ofmul!(::AbstractMatrix, ::Diagonal, ::AbstractMatrix)
. Similar change forAdjoint{<:Any,<:Diagonal}
andTranspose{<:Any,<:Diagonal}
too.Tweak
promote_leaf_eltypes
signature to make the test suite happy. I don't think it will introduce any change for working code. I'll inline-comment on this.