Move toward reusing OrdinaryDiffEqCore infrastructure#75
Conversation
Replace custom IntegratorStats with SciMLBase.DEStats and move tstops/saveat/callback/advance_to_tstop off the top-level integrator struct into a DEOptions (OrdinaryDiffEqCore v4) via a new build_split_deoptions helper. This aligns OperatorSplittingIntegrator more closely with the broader OrdinaryDiffEq integrator interface. SplitSubIntegrator retains its own lightweight IntegratorOptions.
termi-official
left a comment
termi-official
left a comment
There was a problem hiding this comment.
I think the most critical piece missing here is using the new tstops infrastructure from OrdinaryDiffEqCore v4 from SciML/OrdinaryDiffEq.jl#2869 and SciML/OrdinaryDiffEq.jl#3441 .
| return DEOptions( | ||
| 1_000_000, # maxiters | ||
| save_everystep, | ||
| adaptive, | ||
| nothing, # abstol | ||
| nothing, # reltol | ||
| QT(failfactor), | ||
| tType(dtmax), | ||
| tType(dtmin), | ||
| DiffEqBase.ODE_DEFAULT_NORM, | ||
| LinearAlgebra.opnorm, | ||
| nothing, # save_idxs | ||
| tstops, saveat, d_discontinuities, | ||
| tstops_cache, saveat_cache, d_discontinuities_cache, | ||
| nothing, # userdata | ||
| false, 0, "ODE", # progress, progress_steps, progress_name | ||
| DiffEqBase.ODE_DEFAULT_PROG_MESSAGE, | ||
| :ode, # progress_id | ||
| true, false, # timeseries_errors, dense_errors | ||
| nothing, false, # delta, dense | ||
| save_on, save_start, save_end, | ||
| false, false, # save_noise, save_discretes | ||
| nothing, # save_end_user | ||
| callback, | ||
| isoutofdomain, | ||
| DiffEqBase.ODE_DEFAULT_UNSTABLE_CHECK, | ||
| verbose, | ||
| false, false, # calck, force_dtmin | ||
| advance_to_tstop, | ||
| false, # stop_at_next_tstop | ||
| ) |
There was a problem hiding this comment.
Same as above. Most fields do not apply here.
| tType = typeof(dt) | ||
|
|
||
| (!isadaptive(alg) && adaptive && verbose) && | ||
| (!isadaptive(alg) && adaptive && (verbose isa Bool ? verbose : true)) && |
There was a problem hiding this comment.
_inner_verbose goes Bool to DEVerbosity, but here I need a Bool out (the && guard and IntegratorOptions.verbose::Bool). Want a sibling _outer_verbose that does the reverse, or should I just require Bool at the API and let callers wrap?
There was a problem hiding this comment.
Oh, I see. Just FYI we want to integrate later with the verbosity system, because the bool turned out to be quite a mess in the past (because we can just print all or nothing).
| # Time helpers | ||
| tdir(integrator) = | ||
| integrator.tstops.ordering isa DataStructures.FasterForward ? 1 : -1 | ||
| tdir(integrator::OperatorSplittingIntegrator) = integrator.tdir |
There was a problem hiding this comment.
Is this now consistent?
There was a problem hiding this comment.
Yes. The field is set once in __init as tType(tstops_internal.ordering isa FasterForward ? 1 : -1), which is exactly what the old dynamic tdir returned.
|
Needs rebase |
LLM-generated analysis follows, per the suggestion above. I went through OrdinaryDiffEqCore v4's Ideal design
OrdinaryDiffEqCore machinery we should adopt
What stays splitting-specificThe child tree ( Concrete gaps (current vs ideal)
Phased path
Happy to do A here. |
|
Not sure what the best path forward is. Initially I wanted to design the package in a way that reuses Core similar to DelayDiffEq direcly using the stepping logic similar to (https://github.com/SciML/OrdinaryDiffEq.jl/blob/master/lib/DelayDiffEq/src/integrators/interface.jl). @ChrisRackauckas is this the way to go or should we keep the custom stepping infrastructure here for the outer integrator? The curent struct is here OrdinaryDiffEqOperatorSplitting.jl/src/integrator.jl Lines 123 to 179 in 23f5c9b The main difficulty I faced so far for the intermediate subintegrators in the tree ( OrdinaryDiffEqOperatorSplitting.jl/src/integrator.jl Lines 33 to 105 in 23f5c9b
I cannot comprehend this decision. There is some overlap in the options, but the full set of options is disjoint, as we may introduce OS specific options later on, which should not pollute the struct in Core.
On that point I fully agree that this should be fully adopted in some way. However, we must be careful about the interaction between the snapping logic and the new tstop state-machine to not end up in some corrupt state.
Why is this a problem (other that calling the header/footer code in Core directly)?
Agreed that this must be done, but as a separate PR.
I think we have done this already in the other PR.
I agree that this is also separate for now. |
|
Spent some time reading DelayDiffEq and Core to ground the design questions.
Translating to splitting, the pattern fits:
On your other points: sub-integrators stay custom (no The one capability that doesn't come free is continuous callbacks (they'd need If the structural-duck-typing path is right, happy to take a swing as a follow-up PR. |
|
Thanks for taking a more detailed look on the design.
This was also my initial plan, but I have not adopted it yet for two reasons. First, when initially trying to adopt the design I was unsure about the invariants on the event and solution systems involved for the intermediate integrators. Second, I was (and still am) not 100% sure how to guarantee that new minor releases of Core do not cause this package to break, as we rely on internals this way. At least before OrdinaryDiffEq v7 has been released. With the new release I am still not sure what exactly marks the public interface here and all the invariants the integrator data structure has to fulfil. The additional complexity here is that the integrators interact with each other and thus create new invariants which might conflict with invariants assumed in Core -- which is why I pinged Oscar and Chris above. I hope they will find some time soon to comment on this discussion here.
Correct, but note that the signature is significantly different, which is why I have chosen a different name for now to underline this.
I also think so, but this needs confirmation from one of the OrdinaryDiffEqCore devs. Especially one tricky thing with higher order integrators is that the might integrate in both temporal directions to keep the number of stages small and this really needs some careful thinking here about the tstop integration. Unfortunately a bit of a chicken-egg problem here.
We can also make this for free, but at least on the outer level this is almost always too expensive to perform. Note that we can simply implement the full evaluation of the GenericSplitFunction by walking the function tree and moving views into the leaf function calls. The remaining tricky part which I have not touched here yet is to distinguish between the different eval types (e.g. DAE eval vs ODE eval). Is the update here urgent for you? If not, then I would like to wait for feedback from the Core devs first before moving on. |
Nope not urgent. Just trying to push things along :) |
|
I moved some of the fixes for v7 into a separate PR (#81) so they become already available. |
2 participants
Refs #23.
Replaces the custom
IntegratorStatswithSciMLBase.DEStatsand moveststops/saveat/callback/advance_to_tstopoff the top-levelOperatorSplittingIntegratorstruct into aDEOptions(OrdinaryDiffEqCore v4), via a new
build_split_deoptionshelper. Thisaligns
OperatorSplittingIntegratormore closely with the broaderOrdinaryDiffEq integrator interface.
SplitSubIntegratorretains itsown lightweight
IntegratorOptionsfor now.Summary
IntegratorStats; useSciMLBase.DEStatsfor bothSplitSubIntegratorandOperatorSplittingIntegrator.build_split_deoptionsand store options inDEOptionsinstead of as individual fields on the integrator.
__init/ accessors / tests to go throughintegrator.opts.