Skip to content

Conversation

@dcourteville
Copy link
Contributor

Additional context

Fix for #1233

@oscardssmith
Copy link
Member

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

@dcourteville
Copy link
Contributor Author

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

@oscardssmith
Copy link
Member

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
Copy link
Contributor Author

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

@dcourteville
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@oscardssmith
Copy link
Member

This looks excellent!

@oscardssmith
Copy link
Member

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

@dcourteville
Copy link
Contributor Author

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
Copy link
Member

got it. I can make the release.

Co-authored-by: Oscar Smith <[email protected]>
ITP(), abstol = 0.0, reltol = 0.0)
# ODE solver convention: right is toward integration direction
# ITP solver convention: right is toward increasing t
# Note: different non-linear solvers may have different convention
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They shouldn't

@ChrisRackauckas
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants