Fix left/right rootfinding for backward propagation

[dcourteville]

Additional context

Fix for #1233

[oscardssmith]

Any chance that while fixing this you can get rid of InternalITP and switch over to the SciMLBase version?

[dcourteville]

Do you mean the NonlinearSolve version ? I think that would cause dependency issues.

[oscardssmith]

the reason the dependencies are the way they are is that it used to cause dependency issues, but now you can just load NonlinearSolveBase which doesn't have a DiffEq dep.

[dcourteville]

Oh right I can load BracketingNonlinearSolve and it should work. I will try to do it.

[dcourteville]

The main difference between InternalITP and the NonlinearSolve ITP is that InternalITP ignores abstol and reltol and always find the root to floating point precision. I think we should continue to force floating point for callbacks, but I can't pass abstol=0.0 to the NonlinearSolve ITP as this would throw a DomainError. By the way this means that the documentation for abstol and reltol in ContinuousCallback is wrong since they have no effects.
I could either

• Switch to the NonlinearSolve Bisection, which should work with abstol=0.0
• Modify the NonlinearSolve ITP to work with abstol=0.0

[oscardssmith]

the NonlinearSolveBase version really should be using abstol... Did I remove that by mistake? reltol shouldn't do anything though.

We do want to use ITP since it is dramatically faster. so option 2 sounds like the way to go.

[dcourteville]

I just pushed the ITP replacement, now we just have to wait for next BracketingNonlinearSolve version and update the compat.

[oscardssmith]

This looks excellent!

[oscardssmith]

Test failures look real. are we missing a compat bound to ensure that we are using a new enough version with your bugfix?

[dcourteville]

I think we need to release a new version of BracketingNonlinearSolve first before updating the compat here. Is it enough to increment the version in BracketingNonlinearSolve/Project.toml ?

[oscardssmith]

got it. I can make the release.

[ChrisRackauckas]

They shouldn't

[ChrisRackauckas]

Successive callbacks in same integration step: Test Failed at /home/runner/work/DiffEqBase.jl/DiffEqBase.jl/test/downstream/community_callback_tests.jl:255
  Expression: record == [0, 1, 2]
   Evaluated: Any[0, 0, 0, 0, 0, 0, 0, 0, 0, 0  …  0, 0, 0, 0, 0, 0, 0, 0, 0, 0] == [0, 1, 2]

Stacktrace:
 [1] macro expansion
   @ /opt/hostedtoolcache/julia/1.10.10/x64/share/julia/stdlib/v1.10/Test/src/Test.jl:672 [inlined]
 [2] macro expansion
   @ ~/work/DiffEqBase.jl/DiffEqBase.jl/test/downstream/community_callback_tests.jl:255 [inlined]
 [3] macro expansion
   @ /opt/hostedtoolcache/julia/1.10.10/x64/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
 [4] top-level scope
   @ ~/work/DiffEqBase.jl/DiffEqBase.jl/test/downstream/community_callback_tests.jl:236
┌ Warning: Interrupted. Larger maxiters is needed. If you are using an integrator for non-stiff ODEs or an automatic switching algorithm (the default), you may want to consider using a method for stiff equations. See the solver pages for more details (e.g. https://docs.sciml.ai/DiffEqDocs/stable/solvers/ode_solve/#Stiff-Problems).
└ @ SciMLBase ~/.julia/packages/SciMLBase/pzpcE/src/integrator_interface.jl:626
Successive callbacks in same integration step: Test Failed at /home/runner/work/DiffEqBase.jl/DiffEqBase.jl/test/downstream/community_callback_tests.jl:262
  Expression: record == [2, 1, 0]
   Evaluated: Any[0, 0, 0, 0, 0, 0, 0, 0, 0, 0  …  0, 0, 0, 0, 0, 0, 0, 0, 0, 0] == [2, 1, 0]

Stacktrace:
 [1] macro expansion
   @ /opt/hostedtoolcache/julia/1.10.10/x64/share/julia/stdlib/v1.10/Test/src/Test.jl:672 [inlined]
 [2] macro expansion
   @ ~/work/DiffEqBase.jl/DiffEqBase.jl/test/downstream/community_callback_tests.jl:262 [inlined]
 [3] macro expansion
   @ /opt/hostedtoolcache/julia/1.10.10/x64/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
 [4] top-level scope
   @ ~/work/DiffEqBase.jl/DiffEqBase.jl/test/downstream/community_callback_tests.jl:236
Test Summary:                                   | Pass  Fail  Total     Time
Community Callback Tests                        |    4     2      6  3m04.2s
  dae re-init                                   |    1            1     9.0s
  dae re-init                                   |    1            1     3.0s
  Successive callbacks in same integration step |          2      2     6.9s
ERROR: LoadError: Some tests did not pass: 4 passed, 2 failed, 0 errored, 0 broken.
in expression starting at /home/runner/work/DiffEqBase.jl/DiffEqBase.jl/test/runtests.jl:26

This failing test is a blocker, it's showing that successive callbacks are failing so the solution may not be converging to floating point accuracy.

[dcourteville]

The test error is caused by the sol.left and sol.right not being set properly when the ITP finds the exact root. I'll open a PR on NonlinearSolve to fix it and to use the same left/right convention across all bracketing solvers.
I also found a bunch of other issues independent to this PR, so I'll open separate issues for them:

• Most of the other bracketing solvers (Bisection, Brent, ...) also have inconsistent behavior when they find the exact root.
• A callback with an exact root can be detected twice due to floating point issues.
• The abstoland reltol arguments of ContinuousCallback and VectorContinuousCallback don't do anything (but they might be used to solve the previous issue).

[oscardssmith]

I think this now looks good! Thanks for the perseverance on this!

[dcourteville]

Well there is still some work before callback detection is truly robust 😁. I'm already working on another issue I found while fixing on this one.

[oscardssmith]

Despite that, this is now ready to merge. Right?

[dcourteville]

Yes