-
Notifications
You must be signed in to change notification settings - Fork 42
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
Use 5-arg mul! for matrices #68
Conversation
I tested this on a |
Codecov Report
@@ Coverage Diff @@
## master #68 +/- ##
=========================================
- Coverage 98.62% 94.9% -3.73%
=========================================
Files 8 8
Lines 435 451 +16
=========================================
- Hits 429 428 -1
- Misses 6 23 +17
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #68 +/- ##
==========================================
- Coverage 98.62% 97.67% -0.95%
==========================================
Files 8 8
Lines 435 473 +38
==========================================
+ Hits 429 462 +33
- Misses 6 11 +5
Continue to review full report at Codecov.
|
Alright, this is tested really hard for zero allocations, for the case when the map is a wrapped map of some @chriscoey Does this cover most of your applications? If not, I'd be interested to learn about the uncovered ones. |
@dkarrasch thank you, you're brilliant! I'll let you know in the next couple months as we run benchmarks of our as-yet-unreleased optimization solver that calls methods from IterativeSolvers |
Is this one ready for merge? |
I think it is, but I would love to have @Jutho's eyes on it, just in case. |
4df9758
to
f7abb5f
Compare
My apologies for the delay, I'll try to review before the end of the week. |
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.
Looks great and ready to merge (or almost if you consider my small comments).
One final question, because I have lost track a bit: Are we at this point still using one(T)
and zero(T)
as default values of alpha
and beta
in the other 5-arg mul methods, or did we switch to true
and false
everywhere?
An = A.maps[n] | ||
if An isa FreeMap | ||
mul!(y, An, x, true, true) | ||
else |
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.
One trick I learned recently is to add, in the body of a loop, a statement like
if !(@isdefined z)
z = similar(y)
end
which would then only allocate z
once the first map which is not a FreeMap
is encountered, and thus not allocate at all if all the maps are FreeMap
Yes indeed; I should have remembered. Anyway, I think this is good to merge, aside from maybe the |
Closes #56.
MatrixMap{T} <: WrappedMap{T}
LinearAlgebra
's 5-argmul!
MatrixMap
invariant underadjoint
/transpose