feat: integrate callback saving into integrator init and finalize #521
Conversation
AayushSabharwal
force-pushed
the
as/fix-disc-save
branch
from
3a7ccad to
dd09f9e
Compare
AayushSabharwal
changed the title
|
Can you add a test this is working correctly? |
| @@ -72,6 +73,7 @@ FastBroadcast = "7034ab61-46d4-4ed7-9d0f-46aef9175898" | |||
| LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | |||
| LinearSolve = "7ed4a6bd-45f5-4d41-b270-4a48e9bafcae" | |||
| OrdinaryDiffEq = "1dea7af3-3e70-54e6-95c3-0bf5283fa5ed" | |||
| OrdinaryDiffEqCore = "bbf590c4-e513-4bbe-9b18-05decba2e5d8" | |||
There was a problem hiding this comment.
Is this needed given we load OrdinaryDiffeq?
There was a problem hiding this comment.
I'm pretty sure compat bounds don't work if the package isn't a dependency somewhere. So I added ODECore to extras to be able to add a compat.
There was a problem hiding this comment.
yes we only want to do direct dependencies, anything that isn't direct should get changed. We should ExplicitImports.jl test this package, but that can come later.
MTK is the only one that hooks into this API, so tests need to happen there. The design of this is phrased so that with older MTK, it is a no-op (using the old behavior) and with SciML/ModelingToolkit.jl#3901 all MTK-generated callbacks use this infrastructure. If tests pass there, this works. |
|
Essentially this is not a new feature. Instead, it is fixing old broken infrastructure in a way that is transparent to the user. |
|
Yeah this is needed to unblock the testing for the proper codegen and interface testing. I think we should do per-solver tests directly in the interface down the line, but I think we need to first solidify what that interface is and then lock it down. Otherwise the first cases which use the change are stuck in a loop. |
Requires SciML/SciMLBase.jl#1116, SciML/DiffEqBase.jl#1199 and SciML/OrdinaryDiffEq.jl#2857