Closed oscardssmith closed 7 months ago
Attention: 1 lines
in your changes are missing coverage. Please review.
Comparison is base (
49183f7
) 66.06% compared to head (7df519c
) 24.88%.
Files | Patch % | Lines |
---|---|---|
src/factorization.jl | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Let's be serious...
Why exactly do these errors need to be suppressed and what is the preferred behavior here? At least KLU I can modify, but I'm somewhat confused.
the sciml style is to return error codes rather than throw errors because that way, higher level interfaces can check the results better. for example, this PR was created because I was getting a SingularException from the nonlinear solve that initializes a DAE solve which would have initialized properly if the linear solve had returned with a failing error code rather than erroring.
Surely LinSolve should just wrap the solve calls in try catch then? KLU would be sort of non-conformant to the LinearAlgebra factorization "interface" if it returned error codes instead of exceptional errors.
No, try/catch is really slow and is generally not recommended in any numerical code because it introduces dynamic behavior. In general, mathematical functions shouldn't throw but instead report errors. That's why LAPACK's interface has return codes instead of throws, and SuiteSparse does this as well, Sundials, it's why NaNMath.jl exists, etc. all are setup to not throw because that interferes with the recovery behavior of solvers. Julia's interface throws by default, but it's an important enough feature that check=false
is standard in LinearAlgebra.jl. It's just missing from the SuiteSparse bindings. They are the odd one out here.
Alright that's easy enough to fix, I'll try to do that tonight if this flight isn't any further delayed
Thanks!
UMFPACK already has it :P, I'll check on KLU and then make a PR
Wait. ldiv!
doesn't have a check
keyword for any Julia stdlib solvers. Just the factorization call? Is the factorization supposed to store the option, or is this just to disable checking on the factorization call.
None of the operations should error throw. If ldiv!
has the ability to throw an error then it should have a way to turn that off, or we can directly do a ccall
Any errors? Even OOM?
OOM is acceptable (since an OOM isn't really recoverable anyway)
Yeah things that prevent segfaults, sure that needs to error. But it's the errors that are non-essential. Think for example NaNMath.jl. Julia throws an error if it enters the complex plane but that's not necessary, it could just be a NaN as a message that the solution doesn't exist and then a runtime check for NaNs allows optimization routines to continue. This is why JuMP for example has to translate all math functions to NaNMath functions internally.
Yeah I see at least a few possible throws in LinearAlgebra/cholesky.jl. ldiv!
does not respect check = false
, but I believe most of these checks guard against malformed decompositions anyway.
The codes that at least Cholesky would throw at ldiv! time are:
I'm going to operate under the assumption that these are both acceptable (I imagine similar is true for UMFPACK and KLU). And it's only factorization input issues (singularity, issues with the diagonal, etc) which should be disabled for possible recovery.
I've added support for check = false
to KLU's klu
function. However I can't see a path forward to adding this keyword to ldiv!
. When I load using LinearAlgebra
0 methods of ldiv!
have this keyword. And the contract for ldiv!
is to return the result. There is no way for to return an error code unless you want to use the low level calls in this library (which is fine, they have reasonable wrappers, and I can add more check keywords to them if you desire).
These errors must be caught after the factorization and before the solve using issuccess
. I'm not 100% sure but I believe ldiv!
will not throw numerical errors which should always be found in the factorization call.
Okay, if ldiv! isn't throwing errors then I think we're good?
UMFPACK and KLU both throw errors with singular matrices that are not especially easy to silence
ldiv!
for sparse UMFPACK andklu
do not support passingcheck=false
Checklist
Additional context
Add any other context about the problem here.