-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Added function to compute Diagonal*F.Q' and faster methods for other variants. #41248
Conversation
I don't think this is the right approach. I think we should add *(A::Diagonal, adjB::Adjoint{<:Any,<:Union{LinearAlgebra.QRCompactWYQ,LinearAlgebra.QRPackedQ}}) =
rmul!(copyto!(Array{promote_type(eltype(A), eltype(adjB))}(undef, size(A)...), A), adjB)
*(A::BiTriSym, adjB::Adjoint{<:Any,<:Union{LinearAlgebra.QRCompactWYQ,LinearAlgebra.QRPackedQ}}) =
rmul!(copyto!(Array{promote_type(eltype(A), eltype(adjB))}(undef, size(A)...), A), adjB)
*(adjA::Adjoint{<:Any,<:Union{LinearAlgebra.QRCompactWYQ,LinearAlgebra.QRPackedQ}}, B::Diagonal) =
lmul!(adjA, copyto!(Array{promote_type(eltype(adjA), eltype(B))}(undef, size(B)...), B))
*(adjA::Adjoint{<:Any,<:Union{LinearAlgebra.QRCompactWYQ,LinearAlgebra.QRPackedQ}}, B::BiTriSym) =
lmul!(adjA, copyto!(Array{promote_type(eltype(adjA), eltype(B))}(undef, size(B)...), B)) below julia/stdlib/LinearAlgebra/src/special.jl Lines 283 to 286 in 25efa99
The methods that currently handle |
Even simpler: we already have julia/stdlib/LinearAlgebra/src/diagonal.jl Line 292 in 6dfa690
which should be *(D::Diagonal, adjQ::Adjoint{<:Any,<:Union{QRCompactWYQ,QRPackedQ}}) =
rmul!(Array{promote_type(eltype(D), eltype(adjQ))}(D), adjQ) but perhaps we take the opportunity to make sure we also cover |
Nice! This one seems much simpler. But to make it clear, we are not implementing the function as made in the commit because it only covers a narrow number of cases and takes longer to compute the multiplication, right? |
The function as it is currently in the commit has a very narrow type signature, and it also doesn't mutate its first argument as it should. The idea is to provide |
I modified the code a bit, its also a lot faster now and most probably covers a wide range of type signature too. What are your views about this one? Also can we not use |
In your proposal, you again create a copy of
This will likely lead to method ambiguities. |
Got it! I will implement what you suggested above in diagonal.jl along with other methods then. |
A method for that already exists I guess, its working fine. |
Sure, but I guess it's slow. |
well, actually it seems the methods already exist for all other types i.e |
Yes, indeed. It is slow. |
Right. This can be fixed by my proposals above. I'll explain later why this is. Gotta leave now... |
Codecov Report
@@ Coverage Diff @@
## master #41248 +/- ##
==========================================
+ Coverage 86.15% 87.97% +1.82%
==========================================
Files 350 351 +1
Lines 65991 74690 +8699
==========================================
+ Hits 56854 65711 +8857
+ Misses 9137 8979 -158
Continue to review full report at Codecov.
|
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.
There are a few redundant lines, and then we need tests for the new BiTriSym
methods. Would be also great to see some benchmarks, to make sure we do the right thing.
Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>
Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>
Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>
About Benchmarks, do we have to add them in some file or shall I just post it here as a comment? |
Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>
BENCHMARKS
|
For comparison, what are the times on current master? |
|
Not sure why is it still showing 'Changes requested'? Afaik, I have resolved all the reviews. |
…g#41248) Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>
Closes #38974.
Requesting reviews from @dkarrasch @andreasnoack .