Doctests examples for Linear Algebra library as suggested in #18799 #18846
Conversation
There was a problem hiding this comment.
@SaschaMann, this is fantastic! A few minor comments on the first half of the PR follow. Thanks and best!
| 1 ⋅ ⋅ ⋅ | ||
| 7 2 ⋅ ⋅ | ||
| ⋅ 8 3 ⋅ | ||
| ⋅ ⋅ 9 4 |
There was a problem hiding this comment.
Perhaps replace #e is with # ev is for consistency with the rest of the docstring?
| 1 ⋅ ⋅ ⋅ | ||
| 7 2 ⋅ ⋅ | ||
| ⋅ 8 3 ⋅ | ||
| ⋅ ⋅ 9 4 |
There was a problem hiding this comment.
Likewise in this section, perhaps replace #e is with # ev is for consistency with the rest of the docstring?
| 0 1 0 2 | ||
| 2 0 0 0 | ||
| 0 1 0 0 | ||
| ``` |
There was a problem hiding this comment.
Perhaps
julia> A = [1 2; 3 4]
2×2 Array{Int64,2}:
1 2
3 4
julia> B = [1 1; 1 1]
2×2 Array{Int64,2}:
1 1
1 1
julia> kron(A, B)
4×4 Array{Int64,2}:
1 1 2 2
1 1 2 2
3 3 4 4
3 3 4 4would be easier to grasp?
| A = diagm(rand(5)) + diagm(rand(4),1); #A is really bidiagonal | ||
| factorize(A) #factorize will check to see that A is already factorized | ||
| ```jldoctest | ||
| julia> A = Bidiagonal(ones(5, 5), true) #A is really bidiagonal |
There was a problem hiding this comment.
Calling the Bidiagonal constructor to clarify what's going on is a great idea! The remainder of the example relies on A being an Array though. Perhaps wrap your Bidiagonal constructor call in an Array constructor call (Array(Bidiagonal(ones(5, 5), true)))?
Calling the Bidiagonal constructor makes this example sufficiently clear that the comment may no longer be necessary?
There was a problem hiding this comment.
Ah, I missed the idea behind the second part of the example. I added the Array constructor to it.
| @@ -211,6 +211,25 @@ end | |||
| kron(A, B) | |||
|
|
|||
| Kronecker tensor product of two vectors or two matrices. | |||
|
|
|||
| @@ -640,6 +689,20 @@ end | |||
| nullspace(M) | |||
|
|
|||
| Basis for nullspace of `M`. | |||
|
|
|||
| @@ -9,12 +9,38 @@ end | |||
| Diagonal(A::AbstractMatrix) | |||
|
|
|||
| Constructs a matrix from the diagonal of `A`. | |||
|
|
|||
| """ | ||
| Diagonal(A::AbstractMatrix) = Diagonal(diag(A)) | ||
| """ | ||
| Diagonal(V::AbstractVector) | ||
|
|
||
| Constructs a matrix with `V` as its diagonal. | ||
|
|
| @@ -550,6 +574,21 @@ dot(x::Number, y::Number) = vecdot(x, y) | |||
| ⋅(x,y) | |||
|
|
|||
| Compute the dot product. For complex vectors, the first vector is conjugated. | |||
|
|
|||
|
|
||
| julia> dot(a, b) | ||
| 5 | ||
| ``` |
There was a problem hiding this comment.
Perhaps an example of conjugation in the complex case would be useful, e.g. dot([im, im], [1, 1])?
|
@Sacha0 Most of the already existing examples (both in linalg and in other packages) don't have an example heading, so I intentionally didn't add any. Imo the it's clear from the different formatting and context that something is an example unless there's additional text to it. However, I could add them if you prefer that. Thanks for your comments! I applied the changes you suggested. |
kshyatt
added
docs
| 1 2 | ||
| 3 4 | ||
|
|
||
| julia> B = [1 1; 1 1] |
There was a problem hiding this comment.
might be more interesting if B isn't all ones here
There was a problem hiding this comment.
Apologies, all ones was my suggestion (to make what kron does easy to grasp). Can you think of an equally clear but more exciting example? Best!
There was a problem hiding this comment.
Perhaps [im 1; 1 1] would be slightly more interesting but similarly easy to grasp?
There was a problem hiding this comment.
or in a different spot, but yeah at least one entry not equal to 1.0
There was a problem hiding this comment.
I'll change it to [im 1; 1 -im], that way it's still clear what it does but a bit more interesting.
The changes so far look great!
The fifth item in the documentation documentation's list of conventions (about two pages down) suggests segregating examples under an appropriate heading. (Those conventions were codified only a few months ago, so much of the existing documentation does not follow them. #8966 contains additional discussion on evolution of documentation conventions; evolving towards more thorough, structured documentation seems to be the plan.) That said, as-is is fine by me. Cheers if you prefer to omit example headings, and likewise cheers if you prefer to add them. Whichever stokes your enthusiasm! :) Best! |
|
I just added some more examples. Also decided to add the example headings to all examples in the file. The reasons you gave for the headings were compelling. Tests still pass fine:
|
| @@ -161,6 +161,25 @@ end | |||
|
|
|||
| Compute the Cholesky factorization of a positive definite matrix `A` | |||
| and return the UpperTriangular matrix `U` such that `A = U'U`. | |||
|
|
|||
| **Example** | |||
There was a problem hiding this comment.
If memory serves, # Example produces a section heading whereas **Example** produces generic emphasis? (Also below.)
There was a problem hiding this comment.
Yep, you're right. I should have checked the docs instead of just looking at the existing example headings in the file. My bad, fixed it.
| 2 50 | ||
|
|
||
| julia> cholfact!(A) | ||
| ERROR: InexactError() |
There was a problem hiding this comment.
Perhaps expand with an example of valid usage?
There was a problem hiding this comment.
cholfact() has an example with proper usage. the description of the !-version just refers to cholfact() as well instead of repeting itself, so imo it makes sense to not create duplicate examples, too. The only addition to the description is the mention of the error, which is what this demonstrates.
| 4 3 | ||
| 6 3 | ||
|
|
||
| julia> lufact(A) |
There was a problem hiding this comment.
Duplicate of the equivalent line just below beginning F = ...?
There was a problem hiding this comment.
Missed that, when copy pasting from the REPL. Fixed it.
|
Changed the Example headings to I went through pretty much all the functions in linalg and for the ones that are still without example, I can't come up with anything helpful so I won't add anything after the most recent commit. (Except for fixes or tweaks to what exists of course, if you suggest anything) |
|
Just wanted to note PRs like that are really cool! Glad we have a Sa(s)cha team writing and reviewing this... :-) |
| 2 50 | ||
|
|
||
| julia> cholfact!(A) | ||
| ERROR: InexactError() |
|
@SaschaMann There seems to be a small conflict. Can you please rebase? When that is done I think it can be merged. |
|
Rebased and hope this worked properly. I just saw that in the first commit 5b90333 I added the |
The PR should include all .rst files that (Two minor notes for the future[1]:
[1] No need to integrate these suggestions in this already great PR! I would advocate merging this PR as is. Of course if you are enthusiastic about integrating either of these suggestions, please enjoy! I would suggest doing so in followup PRs, smaller PRs being easier to review and merge rapidly.) Thanks again and best! |
|
@Sacha0 Thanks for the feedback! I might integrate your suggestions later
on if I find some spare time between classes or so.
And thanks again to everyone who helped throughout this PR and the original
issue, I learned quite a lot from it :)
|
|
Barring objections or requests for time, I plan to merge this Wednesday (PDT). Best! |
| # Example | ||
|
|
||
| ```jldoctest | ||
| julia> A = [1 0 2 0 3; 0 4 0 5 0; 6 0 7 0 8; 0 9 0 1 0; 2 0 3 0 4] |
There was a problem hiding this comment.
probably better to use an example that's complex to make the distinction clearer?
|
Improved the example for |
|
Thanks for the great work @SaschaMann! |
Issue #18799
Added Doctests
cholfact(A)kron(A, B)pinv(M)nullspace(M)Diagonal(A::AbstractMatrix), Diagonal(V::AbstractVector)norm(v), norm(v, Inf), norm(A, Inf)dot(a, b)inv(M)qr(v)svd(A), svdvals(A)SymTridiagonal(dv, ev), Tridiagonal(dl, d, du),Tridiagonal(A)Rewritten examples
Bidiagonal()Symmetric()Hermitian()I have rewritten these existing examples to be easier to understand without having to run the code.
make docs && make -C doc doctestruns fine, no failures forstdlib/linalg. The failures it shows are identical to the ones on the master branch.First PR, so please let me know if I did something wrong :)