-
Notifications
You must be signed in to change notification settings - Fork 64
fast quadratic form for dense matrix, sparse vectors #640
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
Conversation
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.
The doubly layered case (transpose of wrapper of dense) begs the question if that really occurs, and if yes, then what about the adjoint of wrappers of dense? Conversely, would it be too generic to have
function dot(a::AbstractSparseVector, Q::AbstractMatrix, b::AbstractSparseVector)
return _dot_quadratic_form(a, Q, b)
end
ADD: It would actually correspond nicely to the existing
function dot(x::AbstractVector{T1}, A::AbstractSparseMatrixCSC{T2}, y::AbstractVector{T3}) where {T1,T2,T3}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #640 +/- ##
==========================================
- Coverage 84.01% 84.00% -0.02%
==========================================
Files 12 12
Lines 9258 9288 +30
==========================================
+ Hits 7778 7802 +24
- Misses 1480 1486 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
that's the problem with method ambiguities, there is no way to prove Aqua that this is never the case |
About the suggestion of
The issue is precisely because this needs to be disambiguated again |
But we have function dot(x::SparseVector, A::AbstractSparseMatrixCSC, y::SparseVector) If you change it to function dot(x::AbstractSparseVector, A::AbstractSparseMatrixCSC, y::AbstractSparseVector) that would be the disambiguating method, wouldn't it? |
Making it general was making a whole lot of ambiguities pop: |
Generalizing SparseVector to AbstractSparse in the other method worked though :) |
ok all settled on my end, @dkarrasch can you take a look to see if things are all good for you? |
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.
I believe we need these. The other use case is actually wrong. I'll need to fix it, and then backport the fix. Could you perhaps double check for one of the examples that we actually call the right method?
Co-authored-by: Daniel Karrasch <[email protected]>
Co-authored-by: Daniel Karrasch <[email protected]>
OK something interesting, there is an incorrect behavior somewhere: using SparseArrays, LinearAlgebra, Random
Random.seed!(42)
A = sprand(ComplexF64, 10, 15, 0.4)
Av = view(A, :, :)
x = sprand(ComplexF64, 10, 0.5)
y = sprand(ComplexF64, 15, 0.5)
@test dot(y, Symmetric(collect(A)' * collect(A)), y) ≈ dot(y, Symmetric(A' * A), y) fails on the current SparseArrays default (on 1.11) |
Specifically this function: |
The test will fail if we haven't overwritten the function |
Hm, this is another bug in the method signature: dot(x::SparseVector, A::RealHermSymComplexHerm{<:Any,<:AbstractSparseMatrixCSC}, y::SparseVector) needs to have |
A fix is coming in #643. Let's merge that one first, then update here and make sure we don't end up with ambiguities. |
The tests are passing, good to merge I'd say |
This was missing from the specializations and thus still slow.
This is a port from the same PR on FW.jl: ZIB-IOL/FrankWolfe.jl#607