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.

Sign up for GitHub

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

Make generic cholesky throw on non-psd input #49417

Merged
merged 1 commit into from
Apr 21, 2023
Merged

Make generic cholesky throw on non-psd input #49417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions stdlib/LinearAlgebra/src/cholesky.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ function _chol!(A::AbstractMatrix, ::Type{UpperTriangular})
A[k,k] = Akk
Akk, info = _chol!(Akk, UpperTriangular)
if info != 0
return UpperTriangular(A), info
return UpperTriangular(A), convert(BlasInt, k)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why k here instead of info? Same below

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that the number (if nonzero) is supposed to indicate in which step/row/column the respective issue occurred, but I may have mixed that up with lu, and could reverse that here.

Copy link
Contributor

Choose a reason for hiding this comment

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

was just a curious / non-obvious change from what was previously there

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, this is what it says in lapack.jl within potrf!

#info[] > 0 means the leading minor of order info[] is not positive definite
#ordinarily, throw Exception here, but return error code here
#this simplifies isposdef! and factorize

end
A[k,k] = Akk
AkkInv = inv(copy(Akk'))
Expand All @@ -233,7 +233,7 @@ function _chol!(A::AbstractMatrix, ::Type{LowerTriangular})
A[k,k] = Akk
Akk, info = _chol!(Akk, LowerTriangular)
if info != 0
return LowerTriangular(A), info
return LowerTriangular(A), convert(BlasInt, k)
end
A[k,k] = Akk
AkkInv = inv(Akk)
Expand All @@ -251,11 +251,12 @@ function _chol!(A::AbstractMatrix, ::Type{LowerTriangular})
end

## Numbers
function _chol!(x::Number, uplo)
function _chol!(x::Number, _)
rx = real(x)
iszero(rx) && return (rx, convert(BlasInt, 1))
rxr = sqrt(abs(rx))
rval = convert(promote_type(typeof(x), typeof(rxr)), rxr)
rx == abs(x) ? (rval, convert(BlasInt, 0)) : (rval, convert(BlasInt, 1))
return (rval, convert(BlasInt, rx != abs(x)))
end

## for StridedMatrices, check that matrix is symmetric/Hermitian
Expand Down
29 changes: 16 additions & 13 deletions stdlib/LinearAlgebra/test/cholesky.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,29 +260,32 @@ end
end
end

@testset "behavior for non-positive definite matrices" for T in (Float64, ComplexF64)
@testset "behavior for non-positive definite matrices" for T in (Float64, ComplexF64, BigFloat)
A = T[1 2; 2 1]
B = T[1 2; 0 1]
C = T[2 0; 0 0]
# check = (true|false)
for M in (A, Hermitian(A), B)
for M in (A, Hermitian(A), B, C)
@test_throws PosDefException cholesky(M)
@test_throws PosDefException cholesky!(copy(M))
@test_throws PosDefException cholesky(M; check = true)
@test_throws PosDefException cholesky!(copy(M); check = true)
@test !LinearAlgebra.issuccess(cholesky(M; check = false))
@test !LinearAlgebra.issuccess(cholesky!(copy(M); check = false))
end
for M in (A, Hermitian(A), B)
@test_throws RankDeficientException cholesky(M, RowMaximum())
@test_throws RankDeficientException cholesky!(copy(M), RowMaximum())
@test_throws RankDeficientException cholesky(M, RowMaximum(); check = true)
@test_throws RankDeficientException cholesky!(copy(M), RowMaximum(); check = true)
@test !LinearAlgebra.issuccess(cholesky(M, RowMaximum(); check = false))
@test !LinearAlgebra.issuccess(cholesky!(copy(M), RowMaximum(); check = false))
C = cholesky(M, RowMaximum(); check = false)
@test_throws RankDeficientException chkfullrank(C)
C = cholesky!(copy(M), RowMaximum(); check = false)
@test_throws RankDeficientException chkfullrank(C)
if T !== BigFloat # generic pivoted cholesky is not implemented
for M in (A, Hermitian(A), B)
@test_throws RankDeficientException cholesky(M, RowMaximum())
@test_throws RankDeficientException cholesky!(copy(M), RowMaximum())
@test_throws RankDeficientException cholesky(M, RowMaximum(); check = true)
@test_throws RankDeficientException cholesky!(copy(M), RowMaximum(); check = true)
@test !LinearAlgebra.issuccess(cholesky(M, RowMaximum(); check = false))
@test !LinearAlgebra.issuccess(cholesky!(copy(M), RowMaximum(); check = false))
C = cholesky(M, RowMaximum(); check = false)
@test_throws RankDeficientException chkfullrank(C)
C = cholesky!(copy(M), RowMaximum(); check = false)
@test_throws RankDeficientException chkfullrank(C)
end
end
@test !isposdef(A)
str = sprint((io, x) -> show(io, "text/plain", x), cholesky(A; check = false))
Expand Down