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

ldiv! ambiguous for fully blocked problem #319

Open
termi-official opened this issue Nov 9, 2023 · 5 comments
Open

ldiv! ambiguous for fully blocked problem #319

termi-official opened this issue Nov 9, 2023 · 5 comments

Comments

@termi-official
Copy link

MWE on Julia 1.9.3 with latest release

julia> mortar(reshape([spdiagm([1.0,1.0]), spzeros(2,2), spzeros(2,2), spdiagm([1.0,1.0])], 2,2)) \ Vector(mortar([[1.0,1.0],[1.0,1.0]]))
4-element Vector{Float64}:
 1.0
 1.0
 1.0
 1.0

julia> mortar(reshape([spdiagm([1.0,1.0]), spzeros(2,2), spzeros(2,2), spdiagm([1.0,1.0])], 2,2)) \ mortar([[1.0,1.0],[1.0,1.0]])
ERROR: MethodError: ldiv!(::LinearAlgebra.QRCompactWY{Float64, Matrix{Float64}, Matrix{Float64}}, ::PseudoBlockVector{Float64, Vector{Float64}, Tuple{BlockedUnitRange{Vector{Int64}}}}) is ambiguous.

Candidates:
  ldiv!(A::LinearAlgebra.QRCompactWY{T, M, C} where {M<:AbstractMatrix{T}, C<:AbstractMatrix{T}}, b::AbstractVector{T}) where T<:Union{Float32, Float64, ComplexF32, ComplexF64}
    @ LinearAlgebra ~/Tools/julia-1.9.3/share/julia/stdlib/v1.9/LinearAlgebra/src/qr.jl:873
  ldiv!(A::LinearAlgebra.QRCompactWY, x::ArrayLayouts.LayoutVector; kwds...)
    @ ArrayLayouts ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:153
  ldiv!(A::LinearAlgebra.Factorization, x::ArrayLayouts.LayoutVector; kwds...)
    @ ArrayLayouts ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:150

Possible fix, define
  ldiv!(::LinearAlgebra.QRCompactWY{T, M, C} where {M<:AbstractMatrix{T}, C<:AbstractMatrix{T}}, ::ArrayLayouts.LayoutVector{T}) where T<:Union{Float32, Float64, ComplexF32, ComplexF64}

Stacktrace:
  [1] ldiv!(Y::PseudoBlockVector{Float64, Vector{Float64}, Tuple{BlockedUnitRange{Vector{Int64}}}}, A::LinearAlgebra.QRCompactWY{Float64, Matrix{Float64}, Matrix{Float64}}, B::BlockVector{Float64, Vector{Vector{Float64}}, Tuple{BlockedUnitRange{Vector{Int64}}}})
    @ LinearAlgebra ~/Tools/julia-1.9.3/share/julia/stdlib/v1.9/LinearAlgebra/src/factorization.jl:126
  [2] #_ldiv!#17
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:84 [inlined]
  [3] _ldiv!
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:84 [inlined]
  [4] copyto!(dest::PseudoBlockVector{Float64, Vector{Float64}, Tuple{BlockedUnitRange{Vector{Int64}}}}, M::ArrayLayouts.Ldiv{ArrayLayouts.QRCompactWYLayout{ArrayLayouts.DenseColumnMajor, ArrayLayouts.DenseColumnMajor}, BlockArrays.BlockLayout{ArrayLayouts.DenseColumnMajor, ArrayLayouts.DenseColumnMajor}, LinearAlgebra.QRCompactWY{Float64, Matrix{Float64}, Matrix{Float64}}, BlockVector{Float64, Vector{Vector{Float64}}, Tuple{BlockedUnitRange{Vector{Int64}}}}}; kwds::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ ArrayLayouts ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:113
  [5] copyto!
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:113 [inlined]
  [6] #ldiv!#24
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:104 [inlined]
  [7] ldiv!
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:104 [inlined]
  [8] #_ldiv!#16
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:83 [inlined]
  [9] _ldiv!
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:83 [inlined]
 [10] #copyto!#27
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:113 [inlined]
 [11] copyto!
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:113 [inlined]
 [12] #copy#12
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:21 [inlined]
 [13] copy
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:21 [inlined]
 [14] #materialize#13
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:22 [inlined]
 [15] materialize
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:22 [inlined]
 [16] #ldiv#20
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:98 [inlined]
 [17] ldiv
    @ ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:98 [inlined]
 [18] \(A::BlockMatrix{Float64, Matrix{SparseMatrixCSC{Float64, Int64}}, Tuple{BlockedUnitRange{Vector{Int64}}, BlockedUnitRange{Vector{Int64}}}}, x::BlockVector{Float64, Vector{Vector{Float64}}, Tuple{BlockedUnitRange{Vector{Int64}}}}; kwds::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ ArrayLayouts ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:199
 [19] \(A::BlockMatrix{Float64, Matrix{SparseMatrixCSC{Float64, Int64}}, Tuple{BlockedUnitRange{Vector{Int64}}, BlockedUnitRange{Vector{Int64}}}}, x::BlockVector{Float64, Vector{Vector{Float64}}, Tuple{BlockedUnitRange{Vector{Int64}}}})
    @ ArrayLayouts ~/.julia/packages/ArrayLayouts/RwIbZ/src/ldiv.jl:199
 [20] top-level scope
    @ REPL[17]:1
@dlfivefifty
Copy link
Member

This is actually an issue in ArrayLayouts.jl.

@termi-official
Copy link
Author

Oh, sorry. Didn't noticed. Will open an issue there.

@dlfivefifty
Copy link
Member

No worries! I maintain both so its fine.

I think we just need to add another case here:

https://github.com/JuliaLinearAlgebra/ArrayLayouts.jl/blob/d44e944e3621153f558e919fdeccf6d7c0604266/src/ldiv.jl#L153C26-L153C26

ie

LinearAlgebra.ldiv!(A::LinearAlgebra.QRCompactWY{T}, x::$Typ{T}; kwds...) where T<:BlasFloat = ArrayLayouts.ldiv!(A,x; kwds...)

If you felt like making a PR and checking it works that would be helpful

@termi-official
Copy link
Author

Well, after taking a look into this I think this is a bit harder to fix. Aqua found 2815 ambiguities for this package in total. I picked random samples and they look legit. The ambiguity check is currently disabled ( I think due to the problem with the dependencies in Aqua ambiguitiy check), but this issue can be circumvented by adding the test separately.

Aqua.test_ambiguities(ArrayLayouts) # Must be separate for now, see https://github.com/JuliaTesting/Aqua.jl/issues/77

I will try again to see if I can come up with something.

@termi-official
Copy link
Author

termi-official commented Nov 10, 2023

Okay let us go with the hotfix for now. Trying to fix more ambiguities just made thigns worse.

Edit: Okay, I diretly ran into more dispatches after fixing this. Will include the fixes in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants