Don't subtype AbstractQ <: AbstractMatrix
#46196
Conversation
dkarrasch
added
kind:breaking
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
This came up in a nanosoldier run in JuliaLang/julia#46196. Transposes of Q's don't have specialized functions and fall back to `AbstractMatrix` methods, in particular in multiplication. Generic multiplication, however, is defined in terms of elementwise `getindex`, which is expensive and very slow in contrast to dense or structured matrices.
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
SparseMatrixCSC(::AbstractQ) constructor and multiplication
JuliaSparse/SparseArrays.jl#196
|
This is currently failing only in
Otherwise, this has already lead to the detection of "suboptimal" performance usage, see the list of PRs above. Nanosoldier doesn't run GPU packages, so we may have missed breakage in that part of the ecosystem. @maleadt Would you help me detect packages that would require adjustments, or ping somebody? Given that breakage appears to be very limited, I think this PR has some chance. @ViralBShah @KristofferC How does it work? Is @static if VERSION > v"1.9....DEV..."
AdjointQtype = AdjointQ
else
AdjointQtype = Adjoint
endand then replace the current @nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
This is probably the right thing to do so great that you go through all the work required to make this possible. Part of me thinks it a bit sad that we can't use the matrix abstraction for something like this but, as you point out, |
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. |
dkarrasch
changed the title
AbstractQ <: AbstractMatrixAbstractQ <: AbstractMatrix
|
All green, let's go! |
|
Small comment related to the git history and also the ability to review: IMO it's generally best to split refactoring and functional changes into separate PRs. It's hard to review a diff when functions are also moved to new files. |
|
Thanks for the feedback @andreasnoack. I should have started by first collecting all the existing methods in a new file, and then change the code. |
|
I just noticed that this broke CUDA.jl (and probably other GPU back-ends too). Specifically, our CUSOLVER wrappers testing julia/stdlib/LinearAlgebra/src/qr.jl Lines 839 to 844 in 72aec42 julia/stdlib/LinearAlgebra/src/abstractq.jl Lines 140 to 143 in ea5c9cb convert loses our GPU array data because of julia/stdlib/LinearAlgebra/src/abstractq.jl Line 230 in ea5c9cb Notice the hard-coded julia> Q
15×15 LinearAlgebra.QRPackedQ{Float32, CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}, CuArray{Float32, 1, CUDA.Mem.DeviceBuffer}}
julia> LinearAlgebra.QRPackedQ{T}(Q)
15×15 LinearAlgebra.QRPackedQ{Float32, CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}, Vector{Float32}} |
|
What was the definition of |
|
Oh, or would it already help to |
It used to fall back to I'd also be happy to add a no-op definition for |
|
Yeah, that works: julia> typeof(convert(LinearAlgebra.AbstractQ{T}, Q))
LinearAlgebra.QRPackedQ{Float32, CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}, Vector{Float32}}
julia> LinearAlgebra.QRPackedQ{T}(Q::LinearAlgebra.QRPackedQ) where {T} = LinearAlgebra.QRPackedQ(convert(AbstractMatrix{T}, Q.factors), convert(AbstractVector{T}, Q.τ))
julia> typeof(convert(LinearAlgebra.AbstractQ{T}, Q))
LinearAlgebra.QRPackedQ{Float32, CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}, CuArray{Float32, 1, CUDA.Mem.DeviceBuffer}} |
|
And for equal eltypes this will be a no-op as well, right? While you're at it, could you please tell me which |
|
Nevermind, I found it. |
This is a technically breaking experiment. I believe there is very good reason to not subtype
AbstractQasAbstractMatrix. The main reason is... it isn't! Q's are very different from container-like matrices, they are more like container-backed, function-based linear operators. I believe we've had avoidable issues in the past due to automatically allowing broadcast and indexing, see, for instance, #38972 (comment), and method ambiguities, see the deletions in special.jl in this PR. While this may seem weird to some people, let me point out that we have nice precedence for this:AbstractRotationandFactorization, which both behave like matrices in many ways, but also don't subtypeAbstractMatrix! Here, I went even one step further and introduced a minimalAdjointQ <: AbstractQtype to avoid makingQ'anAbstractMatrixagain. In fact, I believe the same would be reasonable for(R::AbstractRotation)', i.e., to introduce a minimalAdjointRotationtype to get these things out of the matrix dispatch (see the disambiguation methods at the end of givens.jl!).I'm not sure such a change in the type system is acceptable at all, but to the very least, we should be able to detect very bad usage of
AbstractQobjects in aAbstractMatrixcontext (especially like in broadcasting) via a nanosoldier run, but I don't believe this will be widespread, more like by accident.