Store info in Cholesky type #21976
Merged
Store info in Cholesky type #21976
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
and delay throwing for non-positive definiteness
fredrikekre
changed the title
|
I think I will deal with the |
tkelman
reviewed
May 25, 2017
base/linalg/cholesky.jl
Outdated
| @@ -18,6 +18,11 @@ | |||
| # through the Hermitian and Symmetric views or exact symmetric or Hermitian elements which | |||
| # is checked for and an error is thrown if the check fails. | |||
|
|
|||
| # The internal structure is as follows | |||
| # - _chol! return the factor and info without checking positive definiteness | |||
base/linalg/cholesky.jl
Outdated
| @@ -99,30 +113,33 @@ function _chol!(A::AbstractMatrix, ::Type{LowerTriangular}) | |||
| end | |||
| end | |||
| end | |||
| return LowerTriangular(A) | |||
| return LowerTriangular(A), convert(BlasInt, 0) # TODO: If we get here, do we know A is pos. def? | |||
There was a problem hiding this comment.
I'd think so. What could be wrong?
There was a problem hiding this comment.
Okay thanks. Just wanted to make sure, will remove the comment.
There was a problem hiding this comment.
I was paranoid so confirmed with experiments:
for N in (10, 100, 1000), i in 1:1000
A = rand(N, N); B = A'A
for M in (A, B)
C, info = invoke(Base.LinAlg._chol!, Tuple{AbstractMatrix, Type{LowerTriangular}}, copy(M), LowerTriangular)
infoblas = cholfact(Hermitian(M, :L)).info
info == infoblas == 0 || (info > 0 && infoblas > 0) || error()
end
for M in (A, B)
C, info = invoke(Base.LinAlg._chol!, Tuple{AbstractMatrix, Type{UpperTriangular}}, copy(M), UpperTriangular)
infoblas = cholfact(Hermitian(M, :U)).info
info == infoblas == 0 || (info > 0 && infoblas > 0) || error()
end
end
andreasnoack
approved these changes
May 26, 2017
tkelman
reviewed
May 30, 2017
| for T in (Float32, Float64, Complex64, Complex128) | ||
| A = T[1 2; 2 1]; B = T[1, 1] | ||
| C = cholfact(A) | ||
| @show typeof(A), typeof(B), typeof(C.factors) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements #21559 (comment)
The first commit is #21595 (bump on that, its pure code deletion), so when that is merged I will rebase.
Tests pass locally, but there are still things that needs to be changed, for instance