Skip to content

Upgrade Restart API to OrdinaryDiffEqCore 4 #2773

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.

Sign up for GitHub

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

Draft
termi-official wants to merge 12 commits into
base: main
Choose a base branch
from
+57 −13
Draft

Upgrade Restart API to OrdinaryDiffEqCore 4 #2773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
61e66fe
Upgrade Restart API to OrdinaryDiffEqCore 3.4
termi-official Feb 2, 2026
e1eeb89
Move to a dispatch based loader for the controller to keep it extenda…
termi-official Feb 2, 2026
78b5903
Use correct version in Ext
termi-official Feb 2, 2026
5a85411
Use correct lower bound for Core as suggested by Joshua
termi-official Feb 2, 2026
7659188
Apply formatter
termi-official Feb 2, 2026
c52f6b6
Load correct errold.
termi-official Feb 2, 2026
84748c8
Merge branch 'main' into do/upgrade-restart-api
JoshuaLampert Feb 4, 2026
511122e
Add correct HDF5 import to ext
termi-official Feb 4, 2026
3c4662e
Allow the new OrdinaryDiffEqCore version again and format.
termi-official Feb 4, 2026
055a5a2
Merge branch 'main' into do/upgrade-restart-api
JoshuaLampert Feb 4, 2026
21f79e4
Merge branch 'main' into do/upgrade-restart-api
JoshuaLampert Feb 4, 2026
db12ed5
cleanup
JoshuaLampert Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,15 @@ Convex = "f65535da-76fb-5f13-bab9-19810c17039a"
ECOS = "e2685f51-7e38-5353-a97d-a921fd2c8199"
Makie = "ee78f7c6-11fb-53f2-987a-cfe4a2b5a57a"
NLsolve = "2774e3e8-f4cf-5e23-947b-6d7e65073b56"
OrdinaryDiffEqCore = "bbf590c4-e513-4bbe-9b18-05decba2e5d8"
SparseConnectivityTracer = "9f842d2f-2579-4b1d-911e-f412cf18a3f5"

[extensions]
TrixiCUDAExt = "CUDA"
TrixiConvexECOSExt = ["Convex", "ECOS"]
TrixiMakieExt = "Makie"
TrixiNLsolveExt = "NLsolve"
TrixiOrdinaryDiffEqCore = "OrdinaryDiffEqCore"
TrixiSparseConnectivityTracerExt = "SparseConnectivityTracer"

[compat]
Expand Down Expand Up @@ -96,7 +98,7 @@ MuladdMacro = "0.2.4"
NLsolve = "4.5.1"
Octavian = "0.3.28"
OffsetArrays = "1.13"
OrdinaryDiffEqCore = "1.26, 2, 3.0.0 - 3.2.0"
OrdinaryDiffEqCore = "1.26, 2, 3"
P4est = "0.4.12"
Polyester = "=0.7.16, 0.7.18"
PrecompileTools = "1.2"
Expand Down
46 changes: 46 additions & 0 deletions ext/TrixiOrdinaryDiffEqCore.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
module TrixiOrdinaryDiffEqCore

import Trixi: load_controller!
import OrdinaryDiffEqCore: OrdinaryDiffEqCore, PIController, PIDController
import HDF5: attributes

@static if Base.pkgversion(OrdinaryDiffEqCore) >= v"3.4"
import OrdinaryDiffEqCore: PIControllerCache, PIDControllerCache
end

# Support to load controller
function load_controller!(integrator, controller::PIController, file)
if !("time_integrator_qold" in keys(attributes(file)))
error("Missing data in restart file: check the consistency of adaptive time controller with initial setup!")
end
integrator.qold = read(attributes(file)["time_integrator_qold"])
end

function load_controller!(integrator, controller::PIDController, file)
if !("time_integrator_qold" in keys(attributes(file)) ||
!("time_integrator_controller_err" in keys(attributes(file))))
error("Missing data in restart file: check the consistency of adaptive time controller with initial setup!")
end
integrator.qold = read(attributes(file)["time_integrator_qold"])
controller.err[:] = read(attributes(file)["time_integrator_controller_err"])
end
Comment thread
JoshuaLampert marked this conversation as resolved.

@static if Base.pkgversion(OrdinaryDiffEqCore) >= v"3.4"
function load_controller!(integrator, controller::PIControllerCache, file)
if !("time_integrator_qold" in keys(attributes(file)))
error("Missing data in restart file: check the consistency of adaptive time controller with initial setup!")
end
controller.errold = integrator.qold = read(attributes(file)["time_integrator_qold"])
end

function load_controller!(integrator, controller::PIDControllerCache, file)
if !("time_integrator_qold" in keys(attributes(file)) ||
!("time_integrator_controller_err" in keys(attributes(file))))
error("Missing data in restart file: check the consistency of adaptive time controller with initial setup!")
end
controller.dt_factor = integrator.qold = read(attributes(file)["time_integrator_qold"])
controller.err[:] = read(attributes(file)["time_integrator_controller_err"])
end
end

end
Comment on lines +3 to +46
Copy link
Copy Markdown
Member

@DanielDoehring DanielDoehring Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

TBH: This looks like a nightmare to maintain, especially given rather unpredictable and unreliable actions upstream. Maybe we consider dropping the controller completely from the restart file? Opinions @ranocha ?

Copy link
Copy Markdown
Author

@termi-official termi-official Feb 3, 2026

Choose a reason for hiding this comment

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

This looks like a nightmare to maintain [...]

May I ask why? From my perspective this should actually help in terms of adding new controllers in a more reliable way, as you get now a clear error if an unknown controller has been used. It should also be easier now to add custom controllers, as for example done with the PID controller.

Also, there are at least plans to drop the old interface with OrdinaryDiffEq v7 to simplify the controller logic and integration, as it became a bit messy over time with the orgrainical growth. And as I said intitially, I would also like to move this upstream into SciML, as I think having some IO mechanism would be beneficial for other frameworks too. What do you think?

Maybe we consider dropping the controller completely from the restart file?

I am not sure if that is a good idea tbh. Not loading the controller correctly tanked some of the tests quite badly.

[...] especially given rather unpredictable and unreliable actions upstream [...].

Just my two cents here. Since that is a concern to you, it might be worth communicating to the SciML people what makes their actions so bad, so they have at least the chance to adjust their development processes. I am not sure if they are fully aware of what is going so bad in your case.

Copy link
Copy Markdown
Member

@DanielDoehring DanielDoehring Feb 3, 2026

Choose a reason for hiding this comment

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

May I ask why?

Because this essentially requires knowledge of undocumented functionality (which we also use in other parts of the code, which is certainly not ideal)

I am not sure if that is a good idea tbh. Not loading the controller correctly tanked some of the tests quite badly.

Hm yes, but that just amounts to fixing some tests.

Just my two cents here. Since that is a concern to you, it might be worth communicating to the SciML people what makes their actions so bad, so they have at least the chance to adjust their development processes. I am not sure if they are fully aware of what is going so bad in your case.

Hm, green tests would be a starting point?
Screenshot from 2026-02-03 11-01-03

Copy link
Copy Markdown
Author

@termi-official termi-official Feb 4, 2026

Choose a reason for hiding this comment

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

Because this essentially requires knowledge of undocumented functionality

This was already the case before. Now, after the refactoring of the controller interface, you should be able to just de-/serialize the controller cache object in most cases. Please note that I had to keep the old interface and full compatibility with trying to not break too much downstream codes relying on having access to these fields, like for example Trixi . I try to remove the old interface with the next major release of OrdinaryDiffEq tho. Therefore, in the worst case you can use JLD2 for this instead of relying on the given fields of each cache object. Does that make sense?

Copy link
Copy Markdown
Member

@ranocha ranocha Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for cleaning up the controllers in OrdinaryDiffEq.jl - and for taking your time to update our code accordingly, @termi-official! Did I understand correctly that you plan to implement such a store/load functionality in SciML directly? This would be extremely helpful for us, since it would be tested there and we would not have to maintain it ourselves.
If so, I would be fine restricting the version of OrdinaryDiffEq (Core.jl?) we use with Trixi.jl for the moment, and wait for the breaking release including the new interface to save/load the controller.

Copy link
Copy Markdown
Member

@JoshuaLampert JoshuaLampert Feb 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Would you be fine with merging this PR, @ranocha (edit: we can do this after the next breaking release though)?

Copy link
Copy Markdown
Author

@termi-official termi-official Feb 7, 2026

Choose a reason for hiding this comment

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

I would like to wait with the merge briefly.

Copy link
Copy Markdown
Member

@ranocha ranocha Feb 7, 2026

Choose a reason for hiding this comment

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

Ok, thanks. Let me know when we should proceed with this PR.

Copy link
Copy Markdown
Author

@termi-official termi-official Feb 12, 2026

Choose a reason for hiding this comment

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

The latest release should work out of the box again without the fixes presented here. The new PR is SciML/OrdinaryDiffEq.jl#3038 doing the breaking changes to prepare the OrdinaryDiffEq v7 release (and bumping Core to 4). The fix here will then again be needed to load the controller cache correctly on restart.

Copy link
Copy Markdown
Member

@JoshuaLampert JoshuaLampert Feb 12, 2026

Choose a reason for hiding this comment

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

Thanks for the update! Let's try #2782 then.

17 changes: 7 additions & 10 deletions src/callbacks_step/save_restart.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,25 +174,22 @@ function load_adaptive_time_integrator!(integrator, restart_file::AbstractString
# Read context information for controller
h5open(restart_file, "r") do file
# Ensure that the necessary information was saved
if !("time_integrator_qold" in keys(attributes(file))) ||
!("time_integrator_dtpropose" in keys(attributes(file))) ||
(hasproperty(controller, :err) &&
!("time_integrator_controller_err" in keys(attributes(file))))
if !("time_integrator_dtpropose" in keys(attributes(file)))
error("Missing data in restart file: check the consistency of adaptive time controller with initial setup!")
end
# Load data that is required both for PIController and PIDController
integrator.qold = read(attributes(file)["time_integrator_qold"])
integrator.dtpropose = read(attributes(file)["time_integrator_dtpropose"])
# Accept step to use dtpropose already in the first step
integrator.accept_step = true
# Reevaluate integrator.fsal_first on the first step
integrator.reeval_fsal = true
# Load additional parameters for PIDController
if hasproperty(controller, :err) # Distinguish PIDController from PIController
controller.err[:] = read(attributes(file)["time_integrator_controller_err"])
end

load_controller!(integrator, controller, file)
end
end

function load_controller!(integrator, controller, file)
return error("Loading of controller $(typeof(controller)) not implemented.")
end

include("save_restart_dg.jl")
end # @muladd
3 changes: 1 addition & 2 deletions test/test_aqua.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ include("test_trixi.jl")
piracies = (treat_as_own = [Trixi.StartUpDG.RefElemData,
Trixi.StartUpDG.MeshData],),
# exception necessary because StableRNGs.jl is only used in an extension
# OrdinaryDiffEqCore.jl is a hotfix introduced in https://github.dev/trixi-framework/Trixi.jl/pull/2775
stale_deps = (ignore = [:StableRNGs, :OrdinaryDiffEqCore],))
stale_deps = (ignore = [:StableRNGs],))
@test isnothing(check_no_implicit_imports(Trixi,
skip = (Core, Base, Trixi.P4est, Trixi.T8code,
Trixi.EllipsisNotation)))
Expand Down
Loading