Start using termination conditions from DiffEqBase

[utkarsh530]

Requires #203

[utkarsh530]

Based on: SciML/SimpleNonlinearSolve.jl#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]

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%)	⬆️
src/dfsane.jl	100.00% <100.00%> (ø)
src/klement.jl	86.60% <100.00%> (+1.31%)	⬆️
src/lbroyden.jl	90.90% <100.00%> (+0.50%)	⬆️
src/levenberg.jl	98.86% <100.00%> (+0.04%)	⬆️
src/pseudotransient.jl	100.00% <100.00%> (ø)
src/raphson.jl	100.00% <100.00%> (ø)
src/trustRegion.jl	99.37% <100.00%> (+0.01%)	⬆️
src/gaussnewton.jl	78.31% <81.25%> (-1.14%)	⬇️
src/utils.jl	82.63% <70.83%> (-1.99%)	⬇️

... and 1 file with indirect coverage changes

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

[ChrisRackauckas]

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]

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]

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

[ChrisRackauckas]

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]

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

[ChrisRackauckas]

Is this done?

[utkarsh530]

No, not done yet

[utkarsh530]

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