Skip to content
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

Fix pivoted cholesky docstrings #41298

Merged
merged 2 commits into from
Jul 6, 2021
Merged

Fix pivoted cholesky docstrings #41298

merged 2 commits into from
Jul 6, 2021

Conversation

nalimilan
Copy link
Member

The inverse permutation has to be used in the second equation, and the rank has to be taken into account for rank-deficient cases.
This doesn't make a difference in the simple example shown in the docstring, but it does in more complex cases.

(These docs were introduced by #39964)

The inverse permutation has to be used in the second equation,
and the rank has to be taken into account for rank-deficient cases.
This doesn't make a difference in the simple example shown in the docstring,
but it does in more complex cases.
@nalimilan nalimilan added docs This change adds or pertains to documentation linear algebra Linear algebra labels Jun 21, 2021
@nalimilan nalimilan requested a review from dkarrasch June 21, 2021 08:01
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. How about actually adding a rank-deficient example? Something like

julia> X = [1.0, 2.0, 3.0, 4.0]; A = X*X';

julia> F = cholesky(A, Val(true), check = false);

julia> Ur = F.U[1:F.rank, :]; Lr = F.L[:, 1:F.rank]

julia> A[F.p, F.p]  Ur' * Ur  Lr * Lr'
true

? To reduce the doctests, one could remove the full-rank case, since that one is covered in the non-pivoted case already?

@nalimilan
Copy link
Member Author

Good idea. Done.

@dkarrasch dkarrasch merged commit 7409a1c into master Jul 6, 2021
@dkarrasch dkarrasch deleted the nl/cholesky branch July 6, 2021 18:13
KristofferC pushed a commit that referenced this pull request Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants