SciML / NonlinearSolve.jl

High-performance and differentiation-enabled nonlinear solvers (Newton methods), bracketed rootfinding (bisection, Falsi), with sparsity and Newton-Krylov support.
https://docs.sciml.ai/NonlinearSolve/stable/
MIT License
238 stars 42 forks source link

Start using termination conditions from DiffEqBase #208

Closed utkarsh530 closed 1 year ago

utkarsh530 commented 1 year ago

Requires #203

utkarsh530 commented 1 year ago

Based on: https://github.com/SciML/SimpleNonlinearSolve.jl/pull/45

Should we keep the existing interface, i.e., providing options to specify abstol and reltol from the solve? Currently the handling is similar to that in Broyden in SimpleNonlinearSolve.

codecov[bot] commented 1 year ago

Codecov Report

Merging #208 (350fac5) into master (191a237) will increase coverage by 0.49%. The diff coverage is 93.46%.

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   92.42%   92.91%   +0.49%     
==========================================
  Files          19       19              
  Lines        1702     1806     +104     
==========================================
+ Hits         1573     1678     +105     
+ Misses        129      128       -1     
Files Coverage Δ
src/broyden.jl 100.00% <100.00%> (+12.82%) :arrow_up:
src/dfsane.jl 100.00% <100.00%> (ø)
src/klement.jl 86.60% <100.00%> (+1.31%) :arrow_up:
src/lbroyden.jl 90.90% <100.00%> (+0.50%) :arrow_up:
src/levenberg.jl 98.86% <100.00%> (+0.04%) :arrow_up:
src/pseudotransient.jl 100.00% <100.00%> (ø)
src/raphson.jl 100.00% <100.00%> (ø)
src/trustRegion.jl 99.37% <100.00%> (+0.01%) :arrow_up:
src/gaussnewton.jl 78.31% <81.25%> (-1.14%) :arrow_down:
src/utils.jl 82.63% <70.83%> (-1.99%) :arrow_down:

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

ChrisRackauckas commented 1 year ago

Should we keep the existing interface, i.e., providing options to specify abstol and reltol from the solve?

Yes, for simplicity and keeping the same interface. If abstol or reltol are used, that should specify the termination condition. If the termination condition is used, then that allows for a much more detailed specification. If both are specified, then error.

utkarsh530 commented 1 year ago

If both are specified, then error.

I based this check on the SimpleNonlinearSolve.jl behavior:

https://github.com/SciML/SimpleNonlinearSolve.jl/blob/main/src/batched/utils.jl#L17-L20

ChrisRackauckas commented 1 year ago

@avik-pal can you work with @utkarsh530 to rebase this and get it in? Are the termination conditions all uniform after this?

ChrisRackauckas commented 1 year ago

I think this is just about done, but we need to discuss a few last interface issues:

  1. Usually general solver controls go into solve. We should make this a solve keyword argument?
  2. We should make sure abstol and reltol lower in clearly documented ways that match how it's done in SteadyStateDiffEq.jl, i.e. if abstol and/or reltol is given, it defines a TerminationCondition that is sensible, and if abstol and/or reltol is given but also a TerminationCondition is given, it should check compatibility or error that incompatible termination condition combinations were seen.

What this does is effectively allow for the "simple" interface of abstol/reltol that everyone knows and loves to carry over to this domain, while allowing a much more detailed interface to control convergence if desired. However, we need to make sure it's fully uniform before adopting it.

@avik-pal can you chime in.

avik-pal commented 1 year ago

Yeah I agree with the suggestions. The original implementation lived inside DEQs.jl so it was't fleshed out carefully.

ChrisRackauckas commented 1 year ago

Is this done?

utkarsh530 commented 1 year ago

No, not done yet

utkarsh530 commented 1 year ago

@ChrisRackauckas @avik-pal this should be good to merge now.