bring LSMR implementation of jutho/krylovkit.jl#46 up to date #109
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #109 +/- ##
==========================================
- Coverage 85.87% 83.75% -2.13%
==========================================
Files 30 31 +1
Lines 3286 3373 +87
==========================================
+ Hits 2822 2825 +3
- Misses 464 548 +84 ☔ View full report in Codecov by Sentry. |
|
This should now be ready to merge. |
| """ | ||
| LSMR(; orth = KrylovDefaults.orth,atol = KrylovDefaults.tol,btol = KrylovDefaults.tol,conlim = 1/KrylovDefaults.tol, | ||
| maxiter = KrylovDefaults.maxiter,krylovdim = KrylovDefaults.krylovdim,λ = 0.0,verbosity = 0) |
There was a problem hiding this comment.
I think this docstring could use some formatting improvements and a bit of a clean up 😉
src/algorithms.jl
Outdated
| conlim::S | ||
| maxiter::Int | ||
| verbosity::Int | ||
| λ::S |
There was a problem hiding this comment.
Having unicode as names for fields can turn out to be not entirely convenient, since this means there is no alternative to access that field on a machine/terminal that has no support for this. While I also like to use unicode, I would advise to only do that for internal variable names, or in cases where an alternative alias can be provided.
src/linsolve/linsolve.jl
Outdated
There was a problem hiding this comment.
Probably it would be good to have an additional mention to the docstring that LSMR requires a different interface for the function handles, since it needs both the regular as well as the adjoint action.
| # Now use these norms to estimate certain other quantities, | ||
| # some of which will be small near a solution. | ||
| test1 = normr / normb | ||
| test2 = normAr / (normA * normr) | ||
| test3 = inv(condA) | ||
|
|
There was a problem hiding this comment.
I'm a bit confused by the placement of these variables. Is there a reason to define these before the logger, and to then use test1, test2 and test3 below with some unexplained conditions? I think it is a bit more readable to keep that together:
test3 + 1 <= 1 vs inv(condA) + 1 <= 1 seems like it reads better
There was a problem hiding this comment.
Are these different convergence criteria currently used? If not, I would probably streamline them to be compatible with what we use in the other methods, even if this "reduces" the functionality or options to specify the convergence. I will take a stab at this myself.
src/linsolve/lsmr.jl
Outdated
| @info msg | ||
| end | ||
|
|
||
| if 1 + test3 <= 1 |
There was a problem hiding this comment.
Can this be re-expressed as test3 <= eps(one(test3)) ? This is somewhat strange code, because you could easily think that this just means test3 <= 0, which would be a different check.
test/linsolve.jl
Outdated
There was a problem hiding this comment.
I guess you should probably also test a non-square matrix here, since that is the primary use-case of the method?
|
This PR was closed in favor of #111 |
other than change @maartenvd 's code to use the new
apply_normalandapply_adjointsyntax, this PR is equivalent to #46.