RFC: Delay throwing when CHOLMOD factorizations fail #22345

andreasnoack · 2017-06-12T21:02:34Z

Introduce issuccess function to test if a Factorization succeeded.

fredrikekre · 2017-06-12T22:10:01Z

base/linalg/cholesky.jl

@@ -462,7 +467,7 @@ function A_ldiv_B!(C::CholeskyPivoted, B::StridedMatrix)
 end

 function det(C::Cholesky)
-    C.info == 0 || throw(PosDefException(C.info))
+    issuccess(C) || throw(PosDefException(C.info))


perhaps clearer to check isposdef since we throw an PosDefException here? (eg. #22245 )? same below

tkelman · 2017-06-13T12:41:26Z

base/linalg/factorization.jl

+[1.0 0.0; 0.0 -1.0]
+successful: false
+```
+"""


I think this needs to be attached to something

tkelman · 2017-06-13T12:42:43Z

base/sparse/cholmod.jl

+        if s.is_ll == 1
+            return true
+        else
+            return :maybe


wouldn't it be better for this to be type stable if possible?

Yes. It is WIP. I need to figure out the best way to test if an LDLt Factor is PD.

ahk. that branch is covered in tests?

I'm pretty sure it is but a lot of stuff was still broken so there was many test failures. I'll take a look at the coverage when tests pass.

I've updated the tests such that all branches are now covered.

tkelman · 2017-06-13T19:59:15Z

base/linalg/factorization.jl

@@ -16,6 +16,26 @@ macro assertnonsingular(A, info)
   :($(esc(info)) == 0 ? $(esc(A)) : throw(SingularException($(esc(info)))))
 end

+"""
+


no blank line at the start of docstrings

tkelman · 2017-06-13T20:08:17Z

I like the approach. I wonder if it's too much of a pun to dispatch this on the existing success function.

andreasnoack · 2017-06-14T00:29:25Z

I'm fine with overloading success. I don't see how it could cause confusion. The only minor issue is the missing is that most other of our testing functions have and that success(Cmd) is mainly about side effects whereas success(Factorization) would just be checking a state.

tkelman · 2017-06-16T00:23:32Z

base/sparse/cholmod.jl

@@ -1750,17 +1760,27 @@ end

 det(L::Factor) = exp(logdet(L))

-function isposdef(A::SparseMatrixCSC{<:VTypes,SuiteSparse_long})


does the fallback work for this now?

Yes and I believe that https://github.com/JuliaLang/julia/pull/22345/files#diff-f75d450eb1491e3f787ed1ae6b40424fR378 should be exercising that.

Introduce issuccess function to test if a Factorization succeeded. Fixes #22335

Introduce issuccess function to test if a Factorization succeeded. Fixes JuliaLang#22335

tkelman · 2017-06-27T09:51:33Z

base/linalg/bunchkaufman.jl

+issuccess(B::BunchKaufman) = B.info == 0
+
+function Base.show(io::IO, F::BunchKaufman)
+    println(io, summary(F))


this should show the factor as done for lu and cholesky

~~I don't think that is useful for sparse factorizations.~~

Oops. Too fast. The problem here is that constructing the factor is a mess and not that useful.

it's pedagogically useful - isn't there a helper function in lapack for this?

andreasnoack force-pushed the anj/sparsesolve branch from c0468be to c100131 Compare June 12, 2017 21:03

ararslan added the linear algebra Linear algebra label Jun 12, 2017

fredrikekre reviewed Jun 12, 2017

View reviewed changes

andreasnoack changed the title ~~Delay throwing when CHOLMOD factorizations fail~~ WIP: Delay throwing when CHOLMOD factorizations fail Jun 13, 2017

tkelman reviewed Jun 13, 2017

View reviewed changes

base/linalg/factorization.jl

[1.0 0.0; 0.0 -1.0]

successful: false

```

"""

Copy link

Contributor

tkelman Jun 13, 2017

Choose a reason for hiding this comment

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

I think this needs to be attached to something

tkelman reviewed Jun 13, 2017

View reviewed changes

andreasnoack force-pushed the anj/sparsesolve branch 2 times, most recently from 2c76212 to 3d9121c Compare June 13, 2017 15:09

andreasnoack changed the title ~~WIP: Delay throwing when CHOLMOD factorizations fail~~ RFC: Delay throwing when CHOLMOD factorizations fail Jun 13, 2017

tkelman reviewed Jun 13, 2017

View reviewed changes

andreasnoack force-pushed the anj/sparsesolve branch from 3d9121c to 92e56d1 Compare June 13, 2017 20:19

tkelman reviewed Jun 16, 2017

View reviewed changes

Delay throwing when CHOLMOD factorizations fail

164aaaf

Introduce issuccess function to test if a Factorization succeeded. Fixes #22335

andreasnoack force-pushed the anj/sparsesolve branch from 92e56d1 to 164aaaf Compare June 16, 2017 14:44

kshyatt added the error handling Handling of exceptions by Julia or the user label Jun 21, 2017

andreasnoack merged commit 1509716 into master Jun 24, 2017

andreasnoack deleted the anj/sparsesolve branch June 24, 2017 19:18

DrTodd13 pushed a commit to IntelLabs/julia that referenced this pull request Jun 26, 2017

Delay throwing when CHOLMOD factorizations fail (JuliaLang#22345)

a99f24e

Introduce issuccess function to test if a Factorization succeeded. Fixes JuliaLang#22335

tkelman reviewed Jun 27, 2017

View reviewed changes

andreasnoack mentioned this pull request Jun 27, 2017

Printing for BunchKaufman factorizations could be better #22571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Delay throwing when CHOLMOD factorizations fail #22345

RFC: Delay throwing when CHOLMOD factorizations fail #22345

andreasnoack commented Jun 12, 2017

fredrikekre Jun 12, 2017

tkelman Jun 13, 2017

tkelman Jun 13, 2017

andreasnoack Jun 13, 2017

tkelman Jun 13, 2017

andreasnoack Jun 13, 2017

andreasnoack Jun 16, 2017

tkelman Jun 13, 2017

tkelman commented Jun 13, 2017

andreasnoack commented Jun 14, 2017 •

edited

Loading

tkelman Jun 16, 2017

andreasnoack Jun 16, 2017

tkelman Jun 27, 2017

andreasnoack Jun 27, 2017 •

edited

Loading

andreasnoack Jun 27, 2017

tkelman Jun 27, 2017

		@@ -1750,17 +1760,27 @@ end

		det(L::Factor) = exp(logdet(L))

		function isposdef(A::SparseMatrixCSC{<:VTypes,SuiteSparse_long})

RFC: Delay throwing when CHOLMOD factorizations fail #22345

RFC: Delay throwing when CHOLMOD factorizations fail #22345

Conversation

andreasnoack commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jun 13, 2017

andreasnoack commented Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasnoack Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasnoack commented Jun 14, 2017 •

edited

Loading

andreasnoack Jun 27, 2017 •

edited

Loading