-
-
Notifications
You must be signed in to change notification settings - Fork 122
Fix left/right rootfinding for backward propagation #1234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Any chance that while fixing this you can get rid of InternalITP and switch over to the SciMLBase version? |
|
Do you mean the NonlinearSolve version ? I think that would cause dependency issues. |
|
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. |
|
Oh right I can load BracketingNonlinearSolve and it should work. I will try to do it. |
|
The main difference between InternalITP and the NonlinearSolve ITP is that InternalITP ignores
|
|
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. |
|
I just pushed the ITP replacement, now we just have to wait for next BracketingNonlinearSolve version and update the compat. |
|
This looks excellent! |
|
Test failures look real. are we missing a compat bound to ensure that we are using a new enough version with your bugfix? |
|
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 ? |
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They shouldn't
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:26This failing test is a blocker, it's showing that successive callbacks are failing so the solution may not be converging to floating point accuracy. |
Additional context
Fix for #1233