Add cgls-shift algorithm
#853
Conversation
|
| expected_cgls_lanczos_shift_bytes = storage_cgls_lanczos_shift_bytes(m, k, nshifts) | ||
| (x, stats) = cgls_lanczos_shift(Ao, b, shifts) # warmup | ||
| actual_cgls_lanczos_shift_bytes = @allocated cgls_lanczos_shift(Ao, b, shifts) | ||
| @test expected_cgls_lanczos_shift_bytes ≤ actual_cgls_lanczos_shift_bytes ≤ 1.03 * expected_cgls_lanczos_shift_bytes |
There was a problem hiding this comment.
@amontoison I replaced 1.02 used here by 1.03 and it sort of "fix" the tests with Julia 1.6. What do you think?
There was a problem hiding this comment.
Try a bigger linear system ;)
src/cgls_lanczos_shift.jl
Outdated
|
|
||
| #### Input arguments | ||
|
|
||
| * `A`: a linear operator that models a Hermitian matrix of dimension n; |
There was a problem hiding this comment.
A is a rectangular matrix not a square Hermitian one.
src/cgls_lanczos_shift.jl
Outdated
|
|
||
| #### Keyword arguments | ||
|
|
||
| * `M`: linear operator that models a Hermitian positive-definite matrix of size `n` used for centered preconditioning; |
There was a problem hiding this comment.
Many keyword arguments are missing in the signature at the top of the docstring (atol, rtol, ldiv, M, ...).
src/cgls_lanczos_shift.jl
Outdated
| * `check_curvature`: if `true`, check that the curvature of the quadratic along the search direction is positive, and abort if not, unless `linesearch` is also `true`; | ||
| * `atol`: absolute stopping tolerance based on the residual norm; | ||
| * `rtol`: relative stopping tolerance based on the residual norm; | ||
| * `itmax`: the maximum number of iterations. If `itmax=0`, the default number of iterations is set to `2n`; |
There was a problem hiding this comment.
If A is rectangular, it's 2 × dimension(A'*A) in practice. We should verify that it's 2n and not 2m.
src/cgls_lanczos_shift.jl
Outdated
|
|
||
| def_kwargs_cgls_lanczos_shift = (:(; M = I ), | ||
| :(; ldiv::Bool = false ), | ||
| :(; check_curvature::Bool = false), |
There was a problem hiding this comment.
Not needed because A'A + \lambda I is always SPD due to the structure. In CG-LANCZOS-SHIFT, it's just A + \lambda I.
test/test_solvers.jl
Outdated
| @@ -4,8 +4,8 @@ function test_solvers(FC) | |||
| m = div(n, 2) | |||
| Au = A[1:m,:] # Dimension m x n | |||
| Ao = A[:,1:m] # Dimension n x m | |||
| b = Ao * ones(FC, m) # Dimension n | |||
| c = Au * ones(FC, n) # Dimension m | |||
| b = Ao * ones(FC, m) # Dimension m | |||
There was a problem hiding this comment.
Why did you modify the dimension?
There was a problem hiding this comment.
Good work @tmigot!
Can you explain what are the differences in terms of implementation between cgls-lanczos-shift and cg-lanczos-shift when you provide A'A as input?
It seems that you are not doing the products with A and A' so I suppose that it's a trick of the implementation.
We could add a comment about it in the docstring.
I'm curious to also compare the convergence history of cgls-shift and cg-shift on an ill-conditioned LS problem.
|
@tmigot Any update? |
|
Hey @amontoison ! I didn't find the time to figure out the changes to add the preconditioner, so no real update. |
amontoison
force-pushed
the
cgls-shifts
branch
from
c475c6b to
83a3181
Compare
|
@tmigot I think we should do a last pass on this PR and merge without a preconditioner. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #853 +/- ##
==========================================
+ Coverage 94.43% 94.98% +0.54%
==========================================
Files 44 45 +1
Lines 8206 8148 -58
==========================================
- Hits 7749 7739 -10
+ Misses 457 409 -48 ☔ View full report in Codecov by Sentry. |
thanks a lot @amontoison for fixing and merging this!! If you find this interesting I also have the lsqr_shifts? |
|
@tmigot yes, please send it. |
|
@tmigot Should we rename the method |
|
It is a bit like cg_lanczos and cg. This implementation with shift uses the lanczos variant of cgls as well, so I would keep this name. For lsqr_shift, it saves some memory but not as much as hoped. Still, I found the implementation useful in some tests. I will open a PR. |
cgls_lanczos_shift.jlin the foldersrcinclude("cgls_lanczos_shift.jl")insrc/Krylov.jltest_cgls_lanczos_shift.jlin the foldertestinclude("test_cgls_lanczos_shift.jl")intest/runtests.jlCglsLanczosShiftSolverstructure insrc/krylov_solvers.jlexport CglsLanczosShiftSolverinsrc/krylov_solvers.jlKRYLOV_SOLVERSinsrc/krylov_solvers.jlfor (KS, fun, nsol, ...)at the end ofsrc/krylov_solvers.jlCglsLanczosShiftSolverindocs/src/api.md(section Solver Types)@docs cgls_lanczos_shift cgls_lanczos_shift!in the most relevant markdown file ofdocs/src/solversCGLS_LANCZOS_SHIFTintest/test_allocations.jl:cgls_lanczos_shiftintest/test_mp.jlCglsLanczosShiftSolverintest/test_solvers.jlAdd a test for the warm-start feature ofCGLS_LANCZOS_SHIFTintest/test_warm_start.jlCGLS_LANCZOS_SHIFTintest/test_verbose.jlCGLS_LANCZOS_SHIFTindocs/src/preconditioners.mdCGLS_LANCZOS_SHIFTindocs/src/processes.mdCGLS_LANCZOS_SHIFTindocs/src/factorization-free.md(not needed for methods with multiple shifts)CGLS_LANCZOS_SHIFTindocs/src/storage.mdcgls_lanczos_shift.mdin the folderdocs/src/examplesand updatedocs/make.jlCglsLanczosShiftSolverin a note ofdocs/src/inplace.mdif the solver requires the argumentmemoryornshiftssolve!andcgls_lanczos_shift!insrc/krylov_solve.jl