IterativeSolvers: fix MINRES residual field and IDRS keyword forwarding - #1194
Merged
Merged
Conversation
This file contains hidden or 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
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.
Fixes #24.
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Found while checking whether #181 still reproduces. It does not, but two IterativeSolvers algorithms turned out to be broken outright, which is what #24 reports.
MINRES threw on every solve
solve!readcache.cacheval.residualunconditionally.MINRESIterablecalls that fieldresnorm, soIterativeSolversJL_MINRESfailed withFieldError: type IterativeSolvers.MINRESIterable has no field 'residual'for any input. Residual extraction is now per iterable type, which also keeps the existingIDRSIterablespecial case in one place.IDRS threw whenever tolerances were passed as algorithm kwargs
idrs_iterable!takesabstol,reltolandmaxiterpositionally and accepts onlysmoothingandverboseas keywords. The filter droppedidrs_salone, soIterativeSolversJL_IDRS(abstol = 1e-10)forwardedabstola second time as a keyword and failed with aMethodError. All four positionally passed names are now dropped.IterativeSolversJL_IDRS()with no kwargs was unaffected, which is why the existing tests missed it.Test coverage
IterativeSolversJLtestset, where it had been commented out. It passes the fulltest_interfaceon both the real and complex problems plustest_tolerance_update.bicgstablbreaks down on the identityprob1and throwsArgumentError: matrix contains Infs or NaNsout of LAPACK. That is upstream numerics rather than anything wired wrong here, so it is not in scope.gmres_restart, so it could not have caught the IDRS bug.Notes for review
successful_retcode. This extension callsbuild_linear_solutionwithout a retcode, so every IterativeSolvers algorithm reportsReturnCode.Defaultandsuccessful_retcodeis false, while KrylovJL reportsSuccesson the same problem. That is pre-existing and I left it alone deliberately: the extension never checks convergence, so reportingSuccessunconditionally would claim something it has not verified, for example when a solve simply runs out of iterations. Happy to change it if you want that behaviour, but it seemed like its own decision rather than part of a bug fix.LinearSolve.set_b, which no longer exists; with the current cache API, updatingcache.band re-solving gives errors around 5e-18 for CG, GMRES and BICGSTAB. It can be closed as fixed independently of this PR.Verification
IterativeSolversJL_*variants solve correctly, bare and with tolerance kwargs, both on a fresh solve and after abupdate through the cache: 10 of 10 combinations, previously 6.GROUP=Coresuite passes with zero failures.AI Disclosure: Used Opus 5