[documentation] Improve the docstring of SimpleStats #917

amontoison · 2024-10-29T04:39:59Z

https://jso.dev/Krylov.jl/previews/PR917

src/krylov_stats.jl

github-actions · 2024-10-29T05:08:02Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
FletcherPenaltySolver.jl
JSOSolvers.jl
LLSModels.jl
LinearSolve.jl
Percival.jl
RipQP.jl

codecov · 2024-10-29T05:12:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.70%. Comparing base (d1215ab) to head (a7614ae).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #917      +/-   ##
==========================================
+ Coverage   94.17%   94.70%   +0.53%     
==========================================
  Files          45       45              
  Lines        8219     8027     -192     
==========================================
- Hits         7740     7602     -138     
+ Misses        479      425      -54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gdalle

Thanks for documenting this! I made a few suggestions, mostly to refer to SimpleStats in the more complicated stats docstrings and to only document the fields that don't appear in the simple version

gdalle · 2024-10-29T05:52:26Z

src/krylov_stats.jl

+Type for storing statistics returned by the majority of Krylov solvers.
+The fields are as follows:
+- `niter`: The total number of iterations completed by the solver;
+- `solved`: Indicates whether the solver successfully reached convergence (`true` if solved, ``false` otherwise);


Suggested change

- `solved`: Indicates whether the solver successfully reached convergence (`true` if solved, ``false` otherwise);

- `solved`: Indicates whether the solver successfully reached convergence (`true` if solved, `false` otherwise);

Also, convergence according to which criterion? A tolerance I assume?

The answer is quite complex. It depends which shopping condition is triggered.
It could be also related to the callback.

I need to document that since a long time too: #552

gdalle · 2024-10-29T05:53:05Z

src/krylov_stats.jl

+- `solved`: Indicates whether the solver successfully reached convergence (`true` if solved, ``false` otherwise);
+- `inconsistent`: Flags whether the system was detected as inconsistent (i.e., when `b` is not in the range of `A`);
+- `residuals`: A vector containing the residual norms at each iteration;
+- `Aresiduals`: A vector of `Aᴴ`-residual norms at each iteration;


What is Aᴴ?

The adjoint of A Guillaume.
A^T is the transpose and A^H the conjugate hermitian.
In the real case A^H are A^T are the same matrices.

https://jso.dev/Krylov.jl/dev/processes/#Notation

My bad, I'm more used to A' or A* for the adjoint, hence my surprise

I will use A'. I probably expected users to know too many concepts of linear algebra (A^H, consistent, indefinite, singular, singular, dual, etc...).

Some of those were probably explained elsewhere in the docs, like A^H, but yeah the point of all my review comments was getting some clarification in the stats docstring itself.

gdalle · 2024-10-29T05:53:40Z

src/krylov_stats.jl

+- `Aresiduals`: A vector of `Aᴴ`-residual norms at each iteration;
+- `Acond`: An estimate of the condition number of matrix `A`.
+- `timer`: The elapsed time (in seconds) taken by the solver to complete all iterations;
+- `status`: A string indicating the outcome of the solve, providing additional details beyond `solved`.


Can you give an example? What are its possible values?

gdalle · 2024-10-29T05:54:17Z

src/krylov_stats.jl

@@ -47,7 +48,8 @@ function copyto!(dest :: SimpleStats, src :: SimpleStats)
 end

 """
-Type for statistics returned by LSMR. The attributes are:
+Type for storing statistics returned by LSMR.
+The fields are as follows:


Suggested change

The fields are as follows:

The fields are as follows (see [`Krylov.SimpleStats`](@ref)):

Same for the other more sophisticated stats, so that you don't need to document every field again

What is xNorm here?

The norm of the solution x.
You can compute it cheaply for many methods because the Krylov basis is orthonormal.

okay, we should probably explain it here then

gdalle · 2024-10-29T05:56:03Z

src/krylov_stats.jl

@@ -96,7 +98,8 @@ function copyto!(dest :: LsmrStats, src :: LsmrStats)
 end

 """
-Type for statistics returned by CG-LANCZOS, the attributes are:
+Type for storing statistics returned by CG-LANCZOS.
+The fields are as follows:


Suggested change

The fields are as follows:

The fields are as follows (see [`Krylov.SimpleStats`](@ref)):

What is indefinite here?

It means that the matrix is indefinite.
A matrix is indefinite if it has positive and negative eigenvalues.

okay, we should probably explain it here or in a high-level preamble

gdalle · 2024-10-29T05:56:24Z

src/krylov_stats.jl

@@ -134,7 +137,8 @@ function copyto!(dest :: LanczosStats, src :: LanczosStats)
 end

 """
-Type for statistics returned by CG-LANCZOS with shifts, the attributes are:
+Type for storing statistics returned by CG-LANCZOS-SHIFT and CGLS-LANCZOS-SHIFT.
+The fields are as follows:


Suggested change

The fields are as follows:

The fields are as follows (see [`Krylov.SimpleStats`](@ref)):

What is indefinite here?

gdalle · 2024-10-29T05:56:54Z

src/krylov_stats.jl

@@ -174,7 +178,8 @@ function copyto!(dest :: LanczosShiftStats, src :: LanczosShiftStats)
 end

 """
-Type for statistics returned by SYMMLQ, the attributes are:
+Type for storing statistics returned by SYMMLQ.
+The fields are as follows:


Suggested change

The fields are as follows:

The fields are as follows (see [`Krylov.SimpleStats`](@ref)):

What are residualscg, errors and errorscg here?

gdalle · 2024-10-29T05:57:39Z

src/krylov_stats.jl

@@ -221,7 +226,8 @@ function copyto!(dest :: SymmlqStats, src :: SymmlqStats)
 end

 """
-Type for statistics returned by adjoint systems solvers BiLQR and TriLQR, the attributes are:
+Type for storing statistics returned by adjoint systems solvers BiLQR and TriLQR.
+The fields are as follows:


Suggested change

The fields are as follows:

The fields are as follows (see [`Krylov.SimpleStats`](@ref)):

What do the primal and dual suffixes mean here?

You solve two systems here Ax = b and A'y = c. The primal refers to the first system while the dual refers to the second one.

maybe this docstring could link to an explanation of the algorithm itself for these details

It's just that you checked the docstring in the wrong way.
I don't think any user wants to understand how to use a result before knowing how to obtain it. 😉

Nobody will ever use AdjointStats without previously calling bilqr and trilqr, which explain everything and that gives a pointer to this structure: https://jso.dev/Krylov.jl/dev/solvers/as/

fair enough

gdalle · 2024-10-29T05:58:08Z

src/krylov_stats.jl

@@ -258,7 +264,8 @@ end


 """
-Type for statistics returned by the LNLQ method, the attributes are:
+Type for storing statistics returned by the LNLQ method.
+The fields are as follows:


Suggested change

The fields are as follows:

The fields are as follows (see [`Krylov.SimpleStats`](@ref)):

What do the error_ fields mean here?

gdalle · 2024-10-29T05:58:39Z

src/krylov_stats.jl

@@ -298,7 +305,8 @@ function copyto!(dest :: LNLQStats, src :: LNLQStats)
 end

 """
-Type for statistics returned by the LSLQ method, the attributes are:
+Type for storing statistics returned by the LSLQ method.
+The fields are as follows:


Suggested change

The fields are as follows:

The fields are as follows (see [`Krylov.SimpleStats`](@ref)):

What are the new fields here?

amontoison · 2024-10-30T19:36:56Z

@gdalle I agree with you on many point but for specific statistics like LsmrStats, LnlqStats, AdjointStats, etc...
Many details are explained in the docstring of the Krylov method itself.
I don't think that any user will need to understand LnlqStats if he is not using the method that returned this structure.

Note also that the title of the PR is Improve the docstring of SimpleStats.
By lack of time I didn't do the other stuctures yet, so you review of what you don't understand is meningful but not related to the PR.

src/krylov_stats.jl

[documentation] Improve the docstring of SimpleStats

be06c1d

amontoison commented Oct 29, 2024

View reviewed changes

src/krylov_stats.jl Outdated Show resolved Hide resolved

Update src/krylov_stats.jl

a7614ae

gdalle reviewed Oct 29, 2024

View reviewed changes

amontoison commented Oct 30, 2024

View reviewed changes

src/krylov_stats.jl Outdated Show resolved Hide resolved

Update src/krylov_stats.jl

3c16848

amontoison merged commit 83db3f3 into main Oct 30, 2024
29 of 30 checks passed

amontoison deleted the doc_stats branch October 30, 2024 19:46

amontoison mentioned this pull request Oct 30, 2024

Document content of stats #876

Open

	- `solved`: Indicates whether the solver successfully reached convergence (`true` if solved, ``false` otherwise);
	- `solved`: Indicates whether the solver successfully reached convergence (`true` if solved, `false` otherwise);

	The fields are as follows:
	The fields are as follows (see [`Krylov.SimpleStats`](@ref)):

[documentation] Improve the docstring of SimpleStats #917

[documentation] Improve the docstring of SimpleStats #917

Conversation

amontoison commented Oct 29, 2024 • edited Loading

github-actions bot commented Oct 29, 2024 • edited Loading

codecov bot commented Oct 29, 2024

Codecov Report

gdalle left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amontoison Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amontoison Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amontoison Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amontoison commented Oct 30, 2024

amontoison commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

gdalle left a comment •

edited

Loading

amontoison Oct 29, 2024 •

edited

Loading

amontoison Oct 29, 2024 •

edited

Loading

amontoison Oct 29, 2024 •

edited

Loading