resolve ambiguity between \ methods #20970
Conversation
base/sparse/cholmod.jl
Outdated
| @@ -1603,6 +1603,9 @@ Ac_ldiv_B(L::FactorComponent, B::RowVector) = ctranspose(L)\B # ambiguity | |||
| # linalg/factorizations.jl | |||
| (\){T<:VTypes}(L::Factor{T}, b::StridedVector) = Vector(L\convert(Dense{T}, b)) | |||
| (\){T<:VTypes}(L::Factor{T}, B::StridedMatrix) = Matrix(L\convert(Dense{T}, B)) | |||
| # nix ambiguities with (\){T<:BlasReal}(F::Factorization{T}, V::VecOrMat{Complex{T}}) | |||
| (\){T<:Float64}(L::Factor{T}, B::Vector{Complex{T}}) = Vector(L\convert(Dense{T}, B)) | |||
There was a problem hiding this comment.
Is this to include Union{}? If that is the case then I really think we should fix the ambiguity check instead of adding methods for instances that cannot be constructed.
There was a problem hiding this comment.
The ambiguous methods are
\(F::Factorization{T}, B::Union{Array{Complex{T},1}, Array{Complex{T},2}}) where T<:Union{Float32, Float64} in Base.LinAlg at linalg/factorization.jl:36and
\(L::Base.SparseArrays.CHOLMOD.Factor{T}, B::Union{Base.ReshapedArray{T,2,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:DenseArray, DenseArray{T,2}, SubArray{T,2,A,I,L} where L} where I<:Tuple{Vararg{Union{Base.AbstractCartesianIndex, Int64, Range{Int64}},N} where N} where A<:Union{Base.ReshapedArray{T,N,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:DenseArray where N where T, DenseArray} where T) where T<:Union{Complex{Float64}, Float64} in Base.SparseArrays.CHOLMOD at sparse/cholmod.jl:1605with signature intersection
=> Tuple{Base.#\,Base.SparseArrays.CHOLMOD.Factor{T},Array{Complex{T},2}} where T<:Float64which contains the Float64 case I believe? (Removing the type parameter from the method definition and instead specifying Float64 directly does not resolve the ambiguity.) Thoughts? Thanks!
There was a problem hiding this comment.
I shudder to think of what this would mean in practice, but for functions that accept array inputs, I'm beginning to wonder if we should become more systematic in declaring that "container types rule" and that we should get away from dispatching on specific eltypes at the same level of the dispatch hierarchy. Sketch:
\dispatches on container type\has a fallback definition\(L, B) = _ldiv_eltype(L, B)- The only thing you're allowed to dispatch on in
_ldiv_eltypeis the element type, i.e., all methods look like_ldiv_eltype{T<:BlasFloat}(L::AbstractMatrix{T}, B::AbstractVecOrMat{T}) = call_BLAS_method(convert(StridedMatrix{T}, L), convert(StridedVecOrMat{T}, B)).
|
Less wrong and otherwise cleaner version, brought to you by sleep. I am not certain whether and if so how testing this ambiguity directly is possible; an existing method covered the signature in practice. Best! |
Sacha0
force-pushed
the
nixcholmodambigs
branch
from
65e36fb to
6a1a455
Compare
| # linalg/factorizations.jl | ||
| # Explicit typevars are necessary to avoid ambiguities with defs in linalg/factorizations.jl | ||
| # Likewise the two following explicit Vector and Matrix defs (rather than a single VecOrMat) | ||
| (\){T<:Float64}(L::Factor{T}, B::Vector{Complex{T}}) = complex.(L\real(B), L\imag(B)) |
There was a problem hiding this comment.
Could this be
(\)(L::Factor{Float64}, B::Vector{Complex{Float64}}) = complex.(L\real(B), L\imag(B))?
There was a problem hiding this comment.
I wondered the same. Unfortunately the type parameter seems necessary. Thoughts? Thanks!
There was a problem hiding this comment.
Perhaps file as an issue? And then merge this?
|
Thanks all! |
One small step towards reenabling the ambiguity detection test. Best!