-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
RFC: Extract Bunch-Kaufman factors and use them for printing #22601
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
Conversation
test/linalg/bunchkaufman.jl
Outdated
|
|
||
| @testset "(Automatic) Bunch-Kaufman factor of indefinite matrix" begin | ||
| bc1 = factorize(asym) | ||
| # check thactor factorize gives a Bunch-Kaufman |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check that ?
base/linalg/bunchkaufman.jl
Outdated
| LUD, od = LAPACK.syconv!(B.uplo, copy(B.LD), B.ipiv) | ||
| end | ||
| if d == :D | ||
| D = diagm(diag(LUD)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth returning D as a SymTridiagonal to save space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe a general Tridiagonal if there's no such thing as HermTridiagonal...
base/linalg/bunchkaufman.jl
Outdated
| getindex(B::BunchKaufman, d::Symbol) | ||
| Extract the factors of the Bunch-Kaufman factorization `B`. The factorization can take the | ||
| two forms `L*D*L.'` or `U*D*U.'` where `L` is a `UnitLowerTriangular` matrix, `U` is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's also a hermitian version where it's L*D*L' right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and it is also tested. I'll add it to the docstring.
| return eye(T, n)[:,invperm(B[:p])] | ||
| elseif d == :L || d == :U || d == :D | ||
| if B.rook | ||
| # syconvf_rook just added to LAPACK 3.7.0. Uncomment and remove error when we distribute LAPACK 3.7.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve BunchKaufman printing
base/linalg/bunchkaufman.jl
Outdated
| p = T[1:maxi;] | ||
| uploL = uplo == 'L' | ||
| i = uploL ? 1 : maxi | ||
| # if uplo == 'U' we construct the permution backwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
permutation
| - `:p`: permutation vector | ||
| - `:P`: permutation matrix | ||
| ```jldoctest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Examples
```jldoctest
base/linalg/bunchkaufman.jl
Outdated
| symmetric or Hermitian matrix with 1x1 or 2x2 blocks. The argument `d` can be | ||
| - `:D`: the block diagonal matrix | ||
| - `:L`: the lower triangular factor (if factorization is `L*D*L.'`) | ||
| - `:U`: the lower triangular factor (if factorization is `U*D*U.'`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes more sense to list ' form before .' form
| function Base.show(io::IO, mime::MIME{Symbol("text/plain")}, B::BunchKaufman) | ||
| println(io, summary(B)) | ||
| print(io, "successful: $(issuccess(B))") | ||
| println(io, "D factor:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test the show method and/or fix the relevant doctest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've adjusted the doctest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's an existing one that was showing the raw fields of a BunchKaufman, came up in one of Kristoffer's recent pr's - this will make it fail in a different way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I've fixed that one as well.
b3228a9 to
fe131e4
Compare
doc/src/manual/linear-algebra.md
Outdated
| 2.0 -1.0 -3.0 | ||
| -4.0 -3.0 5.0 | ||
| julia> B = [1.5 2 -4; 2 -1 -3; -4 -3 5] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unintentionally duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed
As part of adding tests, I've also cleaned up the signatures of
bkfactsimilarly to the changes we have made forcholfact. That should actually go in a separate PR so I'll open that shortly.I've also wrapped the LAPACK routine for extracting the factors when using rook pivoting but I realized that they were only added in LAPACK 3.7.0 so we'll have to wait until we bump OpenBLAS before we can enable that functionality. For now, the call to those routines are commented out and an error is thrown.
Fixes JuliaLang/LinearAlgebra.jl#441
Update: The printing looks like