IterativeSolvers: fix MINRES residual field and IDRS keyword forwarding

Author: AJ0070

ext/LinearSolveIterativeSolversExt.jl

@@ -106,7 +106,18 @@ function LinearSolve.init_cacheval(
         history = IterativeSolvers.ConvergenceHistory(partial = true)
         history[:abstol] = abstol
         history[:reltol] = reltol
-        filter_kwargs(; idrs_s = 0, kwargs...) = kwargs
+        # `idrs_iterable!` takes the tolerances and the iteration cap
+        # positionally and accepts only `smoothing`/`verbose` as keywords, so
+        # every name passed positionally below has to be dropped here. Filtering
+        # `idrs_s` alone meant `IterativeSolversJL_IDRS(abstol = ...)` forwarded
+        # `abstol` as a keyword too and failed with a `MethodError`
+        # (SciML/LinearSolve.jl#24).
+        function filter_kwargs(;
+                idrs_s = 0, abstol = nothing, reltol = nothing,
+                maxiter = nothing, kwargs...
+            )
+            return kwargs
+        end
         IterativeSolvers.idrs_iterable!(
             history, u, A, b, s, Pl, abstol, reltol, maxiters;
             filter_kwargs(; alg.kwargs...)...
@@ -167,15 +178,22 @@ function SciMLBase.solve!(cache::LinearCache, alg::IterativeSolversJL; kwargs...
         # TODO inject callbacks KSP into solve! cb!(cache.cacheval)
     end
 
-    resid = cache.cacheval isa IterativeSolvers.IDRSIterable ? cache.cacheval.R :
-        cache.cacheval.residual
+    resid = _iterable_residual(cache.cacheval)
     if resid isa IterativeSolvers.Residual
         resid = resid.current
     end
 
     return SciMLBase.build_linear_solution(alg, cache.u, resid, nothing; iters = i)
 end
 
+# IterativeSolvers does not name this field consistently across its iterables.
+# Reading `.residual` unconditionally made `IterativeSolversJL_MINRES` throw a
+# `FieldError` on every solve, because `MINRESIterable` calls it `resnorm`
+# (SciML/LinearSolve.jl#24).
+_iterable_residual(iterable) = iterable.residual
+_iterable_residual(iterable::IterativeSolvers.IDRSIterable) = iterable.R
+_iterable_residual(iterable::IterativeSolvers.MINRESIterable) = iterable.resnorm
+
 purge_history!(iter, x, b) = nothing
 function purge_history!(iter::IterativeSolvers.GMRESIterable, x, b)
     iter.k = 1

test/Core/basictests.jl

@@ -732,15 +732,40 @@ end
                     ("GMRES", IterativeSolversJL_GMRES(; kwargs...)),
                     ("IDRS", IterativeSolversJL_IDRS(; kwargs...)),
                     ("IDRS(2)", IterativeSolversJL_IDRS(; idrs_s = 2, kwargs...)),
+                    ("MINRES", IterativeSolversJL_MINRES(; kwargs...)),
+                    # BICGSTAB stays out: IterativeSolvers' own bicgstabl breaks
+                    # down on the identity `prob1` here and throws
+                    # "matrix contains Infs or NaNs" out of LAPACK, which is an
+                    # upstream numerical issue rather than a wiring problem on
+                    # this side.
                     # ("BICGSTAB",IterativeSolversJL_BICGSTAB(; kwargs...)),
-                    # ("MINRES",IterativeSolversJL_MINRES(; kwargs...)),
                 )
                 @testset "$(alg[1])" begin
                     test_interface(alg[2], prob1, prob2)
                     test_interface(alg[2], prob3, prob4)
                     test_tolerance_update(alg[2], prob5, u5)
                 end
             end
+
+            @testset "tolerances as algorithm kwargs (#24)" begin
+                # `idrs_iterable!` takes abstol/reltol/maxiter positionally and
+                # accepts only `smoothing`/`verbose` as keywords, so forwarding
+                # them from `alg.kwargs` used to raise a MethodError. MINRES read
+                # `.residual`, which its iterable calls `resnorm`, and threw a
+                # FieldError on every solve.
+                # Note these solves report `ReturnCode.Default` rather than
+                # `Success`: this extension never passes a retcode to
+                # `build_linear_solution`. That is pre-existing and separate from
+                # what is tested here, so assert on the solution itself.
+                for alg in (
+                        IterativeSolversJL_IDRS(abstol = 1.0e-10, reltol = 1.0e-10),
+                        IterativeSolversJL_MINRES(abstol = 1.0e-10, reltol = 1.0e-10),
+                        IterativeSolversJL_CG(abstol = 1.0e-10, reltol = 1.0e-10),
+                    )
+                    sol = solve(LinearProblem(A5, b5), alg)
+                    @test sol.u ≈ u5 rtol = 1.0e-6
+                end
+            end
         end
     end