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

Resolve all method ambiguities in LinearAlgebra #28749

Merged
merged 3 commits into from Aug 21, 2018

Conversation

tkf
Copy link
Member

@tkf tkf commented Aug 19, 2018

This PR has three commits to:

  1. Add a test using detect_ambiguities to avoid further introduction of method ambiguities within LinearAlgebra. 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.

  2. Simply apply @andreasnoack's suggestion Resolve mul!(::Matrix, ::Diagonal, ::Adjoint) ambiguity #27405 (comment) and define mul!(::AbstractMatrix, ::Diagonal, ::StridedMatrix) instead of mul!(::AbstractMatrix, ::Diagonal, ::AbstractMatrix). Similar change for Adjoint{<:Any,<:Diagonal} and Transpose{<:Any,<:Diagonal} too.

  3. 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.

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

@tkf tkf Aug 19, 2018

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

Copy link
Member Author

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?

@kshyatt kshyatt added the domain:linear algebra Linear algebra label Aug 19, 2018
@StefanKarpinski
Copy link
Sponsor Member

Nice! That is a surprisingly and happily small number of tweaks required.

@andreasnoack andreasnoack merged commit 4498d27 into JuliaLang:master Aug 21, 2018
KristofferC pushed a commit that referenced this pull request Aug 21, 2018
* 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)
@KristofferC KristofferC mentioned this pull request Aug 21, 2018
@Sacha0
Copy link
Member

Sacha0 commented Aug 21, 2018

Thanks @tkf! :)

@tkf
Copy link
Member Author

tkf commented Aug 22, 2018

@Sacha0 Don't you think it's a good timing to merge #28665? 😉

@tkf
Copy link
Member Author

tkf commented Aug 22, 2018

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 recursive=true mean to check Main as well?

@tkf
Copy link
Member Author

tkf commented Aug 22, 2018

I noticed that the issue was already filed #28804

staticfloat pushed a commit that referenced this pull request Aug 24, 2018
* 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
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
* 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)
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
* 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)
@KristofferC KristofferC added kind:bugfix This change fixes an existing bug and removed backport pending 1.0 labels Sep 27, 2018
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:linear algebra Linear algebra kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants