[DO-NOT-MERGE] Test new controller interface

[termi-official]

Looking for potential downstream regressions in SciML/OrdinaryDiffEq.jl#2899 .

[github-actions]

Review checklist

This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.

Purpose and scope

• The PR has a single goal that is clear from the PR title and/or description.
• All code changes represent a single set of modifications that logically belong together.
• No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

• The code can be understood easily.
• Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
• There are no redundancies that can be removed by simple modularization/refactoring.
• There are no leftover debug statements or commented code sections.
• The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

• New functions and types are documented with a docstring or top-level comment.
• Relevant publications are referenced in docstrings (see example for formatting).
• Inline comments are used to document longer or unusual code sections.
• Comments describe intent ("why?") and not just functionality ("what?").
• If the PR introduces a significant change or new feature, it is documented in NEWS.md with its PR number.

Testing

• The PR passes all tests.
• New or modified lines of code are covered by tests.
• New or modified tests run in less then 10 seconds.

Performance

• There are no type instabilities or memory allocations in performance-critical parts.
• If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

• The correctness of the code was verified using appropriate tests.
• If new equations/methods are added, a convergence test has been run and the results
  are posted in the PR.

Created with ❤️ by the Trixi.jl community.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (123b8b1) to head (404bfc4).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
src/callbacks_step/save_restart.jl	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #2648       +/-   ##
==========================================
- Coverage   97.02%   0.00%   -97.02%     
==========================================
  Files         563     563               
  Lines       44225   43991      -234     
==========================================
- Hits        42907       0    -42907     
- Misses       1318   43991    +42673     

Flag	Coverage Δ
unittests	0.00% <0.00%> (-97.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[DanielDoehring]

Is this covered by https://github.com/trixi-framework/Trixi.jl/blob/main/examples/structured_1d_dgsem/elixir_euler_shu_osher_nonuniform_fvO2.jl ?

[ranocha]

Yes - and also https://github.com/search?q=repo%3Atrixi-framework%2FTrixi.jl+RDPK3Sp&type=code&p=1

[termi-official]

The PR on the SciML side is essentially ready for merge. Can someone please trigger the CI again to make sure the tests pass on your end, too?

[JoshuaLampert]

Note that most CI jobs run on julia v1.10 and the [sources] section was introduced in julia v1.11. So the custom branch is ignored and CI runs on the latest released OrdinaryDiffEq.jl version.

[termi-official]

Ah, sorry for not noticing that earlier. I bumped the version and also included the benchmarks now. Are the benchmarks automatically triggered?

[JoshuaLampert]

Ah, sorry for not noticing that earlier. I bumped the version and also included the benchmarks now. Are the benchmarks automatically triggered?

No, benchmarks only run on workflow_dispatch and are therefore not automatically triggered:

Trixi.jl/.github/workflows/benchmark.yml

Lines 3 to 5 in 80bf3ab

	on:
	workflow_dispatch:

[JoshuaLampert]

Thanks for creating the PR to double-check your PR in OrdinaryDiffEq.jl isn't breaking stuff here.
CI failures are probably due to some differences in floating point computations in julia v1.11 vs. julia v1.10.
However, are you sure the tests are using the code from the custom branch? I am honestly not sure because we never use the whole OrdinaryDiffEq.jl, but rather the subpackages. I think Pkg.jl does not pick up the code from the branch for the subpackages. To check that I created a small example locally with the following Project.toml:

[deps]
OrdinaryDiffEq = "1dea7af3-3e70-54e6-95c3-0bf5283fa5ed"
OrdinaryDiffEqTsit5 = "b1df2697-797e-41e3-8120-5422d3b24e4a"

[sources]
OrdinaryDiffEq = {path = "/home/lampert/.julia/dev/OrdinaryDiffEq"}

Then I ran with julia --project=. in that folder:

(test) pkg> instantiate
    Updating registry at `~/.julia/registries/General.toml`
    Updating `~/Desktop/test/Project.toml`
  [1dea7af3] + OrdinaryDiffEq v6.105.0 `~/.julia/dev/OrdinaryDiffEq`
  [b1df2697] + OrdinaryDiffEqTsit5 v1.8.0
    Updating `~/Desktop/test/Manifest.toml`
[...]
  [1dea7af3] + OrdinaryDiffEq v6.105.0 `~/.julia/dev/OrdinaryDiffEq`
  [89bda076] + OrdinaryDiffEqAdamsBashforthMoulton v1.7.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqAdamsBashforthMoulton`
  [6ad6398a] + OrdinaryDiffEqBDF v1.12.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqBDF`
  [bbf590c4] + OrdinaryDiffEqCore v2.1.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqCore`
  [50262376] + OrdinaryDiffEqDefault v1.10.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqDefault`
  [4302a76b] + OrdinaryDiffEqDifferentiation v1.19.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqDifferentiation`
  [9286f039] + OrdinaryDiffEqExplicitRK v1.6.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqExplicitRK`
  [e0540318] + OrdinaryDiffEqExponentialRK v1.10.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqExponentialRK`
  [becaefa8] + OrdinaryDiffEqExtrapolation v1.11.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqExtrapolation`
  [5960d6e9] + OrdinaryDiffEqFIRK v1.18.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqFIRK`
  [101fe9f7] + OrdinaryDiffEqFeagin v1.6.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqFeagin`
  [d3585ca7] + OrdinaryDiffEqFunctionMap v1.7.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqFunctionMap`
  [d28bc4f8] + OrdinaryDiffEqHighOrderRK v1.7.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqHighOrderRK`
  [9f002381] + OrdinaryDiffEqIMEXMultistep v1.9.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqIMEXMultistep`
  [521117fe] + OrdinaryDiffEqLinear v1.8.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqLinear`
  [1344f307] + OrdinaryDiffEqLowOrderRK v1.8.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqLowOrderRK`
  [b0944070] + OrdinaryDiffEqLowStorageRK v1.9.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqLowStorageRK`
  [127b3ac7] + OrdinaryDiffEqNonlinearSolve v1.17.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqNonlinearSolve`
  [c9986a66] + OrdinaryDiffEqNordsieck v1.6.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqNordsieck`
  [5dd0a6cf] + OrdinaryDiffEqPDIRK v1.8.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqPDIRK`
  [5b33eab2] + OrdinaryDiffEqPRK v1.6.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqPRK`
  [04162be5] + OrdinaryDiffEqQPRK v1.6.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqQPRK`
  [af6ede74] + OrdinaryDiffEqRKN v1.7.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqRKN`
  [43230ef6] + OrdinaryDiffEqRosenbrock v1.20.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqRosenbrock`
  [2d112036] + OrdinaryDiffEqSDIRK v1.9.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqSDIRK`
  [669c94d9] + OrdinaryDiffEqSSPRK v1.9.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqSSPRK`
  [e3e12d00] + OrdinaryDiffEqStabilizedIRK v1.8.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqStabilizedIRK`
  [358294b1] + OrdinaryDiffEqStabilizedRK v1.6.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqStabilizedRK`
  [fa646aed] + OrdinaryDiffEqSymplecticRK v1.9.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqSymplecticRK`
  [b1df2697] + OrdinaryDiffEqTsit5 v1.8.0
  [79d7bb75] + OrdinaryDiffEqVerner v1.8.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqVerner`
[...]

(test) pkg> st OrdinaryDiffEqTsit5
Status `~/Desktop/test/Project.toml`
  [b1df2697] OrdinaryDiffEqTsit5 v1.8.0

(test) pkg> st -m OrdinaryDiffEqCore
Status `~/Desktop/test/Manifest.toml`
  [bbf590c4] OrdinaryDiffEqCore v2.1.0 `../../.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqCore`

If I understand correctly, this tells us that it is not using the version of OrdinaryDiffEqTsit5.jl, which comes from the OrdinaryDiffEq.jl specified in the [sources] section because OrdinaryDiffEqTsit5.jl is already in our Project.toml file. This means, all packages, which are listed in the Project.toml are not actually using the code from the branch, but the latest released version. So this is not testing what it should test. Tell me if I am wrong.
If you agree, could you please try another approach, which ensures we use the right code, e.g., something like this, where we explicitly install all packages from the correct branch before instantiating? This means, we need a Pkg.add in the action file for every subpackage of OrdinaryDiffEq.jl we need in our tests (so the ones listed in test/Project.toml). In that case, we can also use julia v1.10 again.

[termi-official]

Indeed, thanks for double checking here. I somehow assumed that the sources mechanism automatically recognizes that the other packages are part of the monorepo, but they don't.

I am investigating this locally right now to take a closer look at the failing tests -- and so I do not need to bind unnecessary CI resources. I will report back later with fixes to the CI.

[termi-official]

After going through the tests locally on 1.11 and 1.10 I suspect for now that the broken tests are the result of the following combination:

1. The norms for some of these problems are precomputed to have a proxy catching regressions.
2. I changed in the PR some of the Rational variables to floats, which naturally leads to a difference in the results. If I did not miss any other bug, then the new results should be slightly more "precise" in the sense we do not truncate the error estimates when converting them to Rational's inside the controller code.

The absolute difference is actually not that big in the failing tests I checked (group parabolic_part1, around 1e-10). However, I also have some trouble making the tests pass locally on 1.10. Actually, the same tests fail locally on my machine using main of Trixi and the latest releases for the OrdinaryDiffEq* packages.

Since I have not yet found a good way to reproduce everything locally, I added a test matrix with three Julia versions focussing on some of the failing tests temporarily. Not sure why the updated ci.yml is not picked up yett tho.

[termi-official]

This branch should be on track again (+- AQUA complaining about the compat). src/callbacks_step/save_restart.jl needs some refactoring once the new controller system is on track. Probably easiest to add dispatches per controller cache to de-/serialize the individual cache.

[JoshuaLampert]

Why do we need to add OrdinaryDiffEq.jl?

[termi-official]

I was lazy and didn't wanted to add all packages manually. This merely ensured that all packages are pulled from the PR to ensure we do not mix the master (or to be specific the PR) versions with the latest releases, which can sometimes cause trouble.

[JoshuaLampert]

But does it ensure that all packages are pulled from the PR? When you add OrdinaryDiffEq.jl like this, it only adds the meta package OrdinaryDiffEq.jl from the PR, but this is neither changed in the PR nor used in the tests. All dependencies of OrdinaryDiffEq.jl including the subpackages in the same monorepo are still resolved to the latest released version (except you explicitly add them like you do for the ones below).

[JoshuaLampert]

You can see that this is happening also here: https://github.com/trixi-framework/Trixi.jl/actions/runs/21030660345/job/60466400781?pr=2648#step:8:297.

[termi-official]

But does it ensure that all packages are pulled from the PR?

You are right, it does not. I thought that sourcing the root is sufficient to automatically pull all the non-explicitly stated deps from the same source. I posted the question on discourse https://discourse.julialang.org/t/best-practice-to-use-sources-on-a-monorepository/135074 . It should not affect this PR tho.

[termi-official]

So up to some typos, there is only the MPI restart failing. I will try to investigate this one tomorrow.

[termi-official]

The related PR is merged now. Thanks for the help!

[JoshuaLampert]

We do need to adapt our restart functionality, right? Because I am pretty sure these kind of errors we now have in CI are caused because the controller is not correctly read from the restart files anymore. Am I right, @termi-official? And if yes, could you please create a PR for that (I think the necessary changes are already in save_restart.jl‎ of this PR)?

[termi-official]

Right. Give me a minute.