Check that matrix is symmetric/Hermitian in dense Cholesky #16799
Conversation
| return chol(Hermitian(A)) | ||
| end | ||
|
|
||
| ## Numbers | ||
| """ |
There was a problem hiding this comment.
Isn't the function signature here wrong?
There was a problem hiding this comment.
Are you talking about the args... part?
There was a problem hiding this comment.
Yeah, does that need to be in the sig?
There was a problem hiding this comment.
Often the documentation signatures cheat a bit for simplification. I think that having the args... part in the docstring would confuse more than help. Maybe you could write that "extra arguments are ignored". However, this is not part of the change in this PR so if we want that, it could be done in a separate PR.
There was a problem hiding this comment.
Sure; I just want to make sure the docs still work! On the other hand I don't think the masses are really clamoring for chol(Number) that we need to worry about it :).
| # supported for the four LAPACK element types. For other types, e.g. BigFloats Val{true} will | ||
| # give an error. It is required that the input is Hermitian (including real symmetric) either | ||
| # through the Hermitian and Symmetric views or exact symmetric or Hermitian elements which | ||
| # is checked then checked and an error is thrown if the check fails. The dispatch |
andreasnoack
force-pushed
the
anj/chol
branch
2 times, most recently
from
66429fe to
ae1fd20
Compare
| @@ -23,24 +47,27 @@ function CholeskyPivoted{T}(A::AbstractMatrix{T}, uplo::Char, piv::Vector{BlasIn | |||
| CholeskyPivoted{T,typeof(A)}(A, uplo, piv, rank, tol, info) | |||
| end | |||
|
|
|||
| function chol!{T<:BlasFloat}(A::StridedMatrix{T}, ::Type{UpperTriangular}) | |||
There was a problem hiding this comment.
So this renames the two-argument chol! method with the upper/lower type second argument, making it no longer available under the chol! name at all, right? I did a quick grep of packages and luckily it looks like no one was calling that in anything that's registered.
|
I pushed a commit here that factors the almost-exactly-repeated (only the function name changes) error message for the non Hermitian case into a common helper function, and has some minor edits in a few comments. We can iterate on that if you disagree with anything in 905c2f4, otherwise I think we're good. |
| @@ -136,8 +136,8 @@ begin | |||
| # Cholesky factor of Matrix with non-commutative elements, here 2x2-matrices | |||
There was a problem hiding this comment.
you took your typo fix out from L120 in the rebase
There was a problem hiding this comment.
Should be fixed again now
and some minor edits to a few comments
This fixes JuliaLang/LinearAlgebra.jl#189 by checking that the input matrix is symmetric/Hermitian before computing the Cholesky factorization. This was already the behavior for sparse matrices but not for dense matrices where we simply ignored either the lower or upper part of the input. The check for symmetry can be avoided by using the
Hermitianimmutable.While making this change, I realized how wild the dense Cholesky constructors have grown. Most of this is because of the
Valdispatch for turning pivoting on and off. It was possible to clean it up a little bit but I think a full cleanup would require removal of theValdispatch one way or another. Eliminating theValhere is beyond the scope of this PR but I've added a comment about this.