-
Notifications
You must be signed in to change notification settings - Fork 55
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
Only preallocated one vector for preconditioners #187
Only preallocated one vector for preconditioners #187
Conversation
@@ -4,7 +4,7 @@ using LinearAlgebra | |||
function wrap_preconditioners(kwargs) | |||
if (haskey(kwargs, :M) && typeof(kwargs[:M]) <: AbstractMatrix) || (haskey(kwargs, :N) && typeof(kwargs[:N]) <: AbstractMatrix) | |||
k = keys(kwargs) | |||
v = Tuple(typeof(arg) <: AbstractMatrix ? PreallocatedLinearOperator(arg) : arg for arg in values(kwargs)) | |||
v = Tuple(typeof(arg) <: AbstractMatrix ? PreallocatedLinearOperator(arg, symmetric=true) : arg for arg in values(kwargs)) |
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.
With GMRES & co., preconditioners could be non-symmetric.
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.
Yes, but we only need M⁻¹ * v
products. I did the same thing with A
for CGS
, DIOM
and DQGMRES
even if A
is not symmetric.
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 see. It's worth adding a comment around your last change to explain that.
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.
With the option symmetric=true
, we preallocate matrix-vector products with A and A' with the same vector. If we don't use / need products with A' it's the good thing to do even if A is not symmetric.
Codecov Report
@@ Coverage Diff @@
## master #187 +/- ##
=======================================
Coverage 97.14% 97.14%
=======================================
Files 28 28
Lines 2483 2483
=======================================
Hits 2412 2412
Misses 71 71
Continue to review full report at Codecov.
|
@@ -4,6 +4,7 @@ using LinearAlgebra | |||
function wrap_preconditioners(kwargs) | |||
if (haskey(kwargs, :M) && typeof(kwargs[:M]) <: AbstractMatrix) || (haskey(kwargs, :N) && typeof(kwargs[:N]) <: AbstractMatrix) | |||
k = keys(kwargs) | |||
# Matrix-vector products with Mᵀ and Nᵀ are not required, we can safely use one vector for products with M / Mᵀ and N / Nᵀ |
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.
Please explain that that's why you set symmetric=true
somewhere. Otherwise, I'm sure we'll forget and puzzle over this in 6 months time.
7ad588d
to
e1c1b84
Compare
Thank you. |
No description provided.