Fix successful return codes for factorization methods

Fixes #630 fixes #532

Author: ChrisRackauckas

ext/LinearSolveRecursiveFactorizationExt.jl

@@ -25,7 +25,7 @@ function SciMLBase.solve!(cache::LinearSolve.LinearCache, alg::RFLUFactorization
         cache.isfresh = false
     end
     y = ldiv!(cache.u, LinearSolve.@get_cacheval(cache, :RFLUFactorization)[1], cache.b)
-    SciMLBase.build_linear_solution(alg, y, nothing, cache)
+    SciMLBase.build_linear_solution(alg, y, nothing, cache; retcode = ReturnCode.Success)
 end
 
 end

src/appleaccelerate.jl

@@ -243,6 +243,11 @@ function SciMLBase.solve!(cache::LinearCache, alg::AppleAccelerateLUFactorizatio
         res = aa_getrf!(A; ipiv = cacheval[1].ipiv, info = cacheval[2])
         fact = LU(res[1:3]...), res[4]
         cache.cacheval = fact
+
+        if !LinearAlgebra.issuccess(fact)
+            return SciMLBase.build_linear_solution(
+                alg, cache.u, nothing, cache; retcode = ReturnCode.Failure)
+        end
         cache.isfresh = false
     end
 
@@ -258,5 +263,5 @@ function SciMLBase.solve!(cache::LinearCache, alg::AppleAccelerateLUFactorizatio
         aa_getrs!('N', A.factors, A.ipiv, cache.u; info)
     end
 
-    SciMLBase.build_linear_solution(alg, cache.u, nothing, cache)
+    SciMLBase.build_linear_solution(alg, cache.u, nothing, cache; retcode = ReturnCode.Success)
 end

src/factorization.jl

@@ -150,7 +150,7 @@ function SciMLBase.solve!(cache::LinearCache, alg::LUFactorization; kwargs...)
 
     F = @get_cacheval(cache, :LUFactorization)
     y = _ldiv!(cache.u, F, cache.b)
-    SciMLBase.build_linear_solution(alg, y, nothing, cache)
+    SciMLBase.build_linear_solution(alg, y, nothing, cache; retcode = ReturnCode.Success)
 end
 
 function do_factorization(alg::LUFactorization, A, b, u)
@@ -203,7 +203,7 @@ function SciMLBase.solve!(cache::LinearSolve.LinearCache, alg::GenericLUFactoriz
         cache.isfresh = false
     end
     y = ldiv!(cache.u, LinearSolve.@get_cacheval(cache, :GenericLUFactorization)[1], cache.b)
-    SciMLBase.build_linear_solution(alg, y, nothing, cache)
+    SciMLBase.build_linear_solution(alg, y, nothing, cache; retcode = ReturnCode.Success)
 end
 
 function init_cacheval(
@@ -953,6 +953,12 @@ A fast factorization which uses a Cholesky factorization on A * A'. Can be much
 faster than LU factorization, but is not as numerically stable and thus should only
 be applied to well-conditioned matrices.
 
+!!! warn
+    `NormalCholeskyFactorization` should only be applied to well-conditioned matrices. As a
+    method it is not able to easily identify possible numerical issues. As a check it is
+    recommended that the user checks `A*u-b` is approximately zero, as this may be untrue
+    even if `sol.retcode === ReturnCode.Success` due to numerical stability issues.
+
 ## Positional Arguments
 
   - pivot: Defaults to RowMaximum(), but can be NoPivot()
@@ -1010,6 +1016,12 @@ function SciMLBase.solve!(cache::LinearCache, alg::NormalCholeskyFactorization;
             fact = cholesky(Symmetric((A)' * A), alg.pivot; check = false)
         end
         cache.cacheval = fact
+
+        if hasmethod(LinearAlgebra.issuccess, Tuple{typeof(fact)}) && !LinearAlgebra.issuccess(fact)
+            return SciMLBase.build_linear_solution(
+                alg, cache.u, nothing, cache; retcode = ReturnCode.Failure)
+        end
+
         cache.isfresh = false
     end
     if issparsematrixcsc(A)
@@ -1021,7 +1033,7 @@ function SciMLBase.solve!(cache::LinearCache, alg::NormalCholeskyFactorization;
     else
         y = ldiv!(cache.u, @get_cacheval(cache, :NormalCholeskyFactorization), A' * cache.b)
     end
-    SciMLBase.build_linear_solution(alg, y, nothing, cache)
+    SciMLBase.build_linear_solution(alg, y, nothing, cache; retcode = ReturnCode.Success)
 end
 
 ## NormalBunchKaufmanFactorization

src/mkl.jl

@@ -219,11 +219,16 @@ function SciMLBase.solve!(cache::LinearCache, alg::MKLLUFactorization;
         res = getrf!(A; ipiv = cacheval[1].ipiv, info = cacheval[2])
         fact = LU(res[1:3]...), res[4]
         cache.cacheval = fact
+
+        if !LinearAlgebra.issuccess(fact)
+            return SciMLBase.build_linear_solution(
+                alg, cache.u, nothing, cache; retcode = ReturnCode.Failure)
+        end
         cache.isfresh = false
     end
 
     y = ldiv!(cache.u, @get_cacheval(cache, :MKLLUFactorization)[1], cache.b)
-    SciMLBase.build_linear_solution(alg, y, nothing, cache)
+    SciMLBase.build_linear_solution(alg, y, nothing, cache; retcode = ReturnCode.Success)
 
     #=
     A, info = @get_cacheval(cache, :MKLLUFactorization)

test/retcodes.jl

@@ -1,4 +1,4 @@
-using LinearSolve, LinearAlgebra, RecursiveFactorization
+using LinearSolve, LinearAlgebra, RecursiveFactorization, Test
 
 alglist = (
     LUFactorization,
@@ -14,12 +14,13 @@ alglist = (
 
 @testset "Success" begin
     for alg in alglist
+        @show alg
         A = [2.0 1.0; -1.0 1.0]
         b = [-1.0, 1.0]
         prob = LinearProblem(A, b)
         linsolve = init(prob, alg())
         sol = solve!(linsolve)
-        @test SciMLBase.successful_retcode(sol.retcode) || sol.retcode == ReturnCode.Default # The latter seems off...
+        @test SciMLBase.successful_retcode(sol.retcode)
     end
 end
 
@@ -39,7 +40,13 @@ lualgs = (
         prob = LinearProblem(A, b)
         linsolve = init(prob, alg)
         sol = solve!(linsolve)
-        @test !SciMLBase.successful_retcode(sol.retcode)
+        if alg isa NormalCholeskyFactorization
+             # This is a known and documented incorrectness in NormalCholeskyFactorization
+             # due to numerical instability in its method that is fundamental.
+            @test SciMLBase.successful_retcode(sol.retcode)
+        else
+            @test !SciMLBase.successful_retcode(sol.retcode)
+        end
     end
 end
 
@@ -62,4 +69,4 @@ rankdeficientalgs = (
         sol = solve!(linsolve)
         @test SciMLBase.successful_retcode(sol.retcode)
     end
-end
+end