Implement stage-level coupling for split integration#1049
Implement stage-level coupling for split integration#1049efaulhaber wants to merge 36 commits intotrixi-framework:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements stage-level coupling for split integration of TotalLagrangianSPHSystems, and refactors the ODE problem parameter p to carry both the semidiscretization and split-integration runtime payload.
Changes:
- Change
semidiscretize/ODEppayload fromSemidiscretizationto aNamedTuplewithp.semiplusp.split_integration_data. - Extend
SplitIntegrationCallbackwithstage_coupling/predict_positionsoptions and stage-time integration support. - Update callbacks/IO/visualization code paths to use
integrator.p.semi/sol.prob.p.semi.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
test/callbacks/info.jl |
Update mock integrator payload to match p.semi access pattern. |
src/visualization/recipes_plots.jl |
Adapt plotting recipe and solution type alias to the new p payload shape. |
src/io/io.jl |
Read semidiscretization/metadata from integrator.p.semi. |
src/general/semidiscretization.jl |
Construct p=(; semi, split_integration_data=nothing) and adjust kick!/drift! signatures accordingly. |
src/callbacks/update.jl |
Use integrator.p.semi. |
src/callbacks/stepsize.jl |
Use integrator.p.semi; broaden callback type check for parametric SplitIntegrationCallback. |
src/callbacks/steady_state_reached.jl |
Use integrator.p.semi. |
src/callbacks/split_integration.jl |
Implement stage coupling + new payload storage under p.split_integration_data. |
src/callbacks/solution_saving.jl |
Use integrator.p.semi. |
src/callbacks/post_process.jl |
Use integrator.p.semi. |
src/callbacks/info.jl |
Use integrator.p.semi. |
src/callbacks/density_reinit.jl |
Use integrator.p.semi. |
ext/TrixiParticlesOrdinaryDiffEqExt.jl |
Adjust extension code to read p.semi. |
docs/src/refs.bib |
Remove JabRef metadata comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1049 +/- ##
==========================================
- Coverage 89.54% 89.52% -0.03%
==========================================
Files 127 127
Lines 9654 9710 +56
==========================================
+ Hits 8645 8693 +48
- Misses 1009 1017 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/run-gpu-tests |
c798ff9 to
d749eb3
Compare
dcf99cf to
741c9a6
Compare
|
@copilot Find the issue with the failing CI runs and suggest a fix. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/run-gpu-tests |
|
Since you are changing the extension can you please address this issue #1047 |
|
/run-gpu-tests |
| # Tell OrdinaryDiffEq that `u` has NOT been modified. | ||
| # Theoretically, the TLSPH part has been modified, but in the FSAL case, | ||
| # the time at the last stage is the same as the step time, so the split integration | ||
| # above is skipped and `u` is not modified at all. | ||
| # Therefore, the derivative at the last stage can be reused for the next step. |
There was a problem hiding this comment.
# Tell OrdinaryDiffEq that `u` was not modified.
# In the FSAL case, the last stage occurs at the step time,
# so the split integration is skipped and `u` remains unchanged.
# Therefore, the derivative at the last stage can be reused for the next step.
| # We modify `v_ode` and `u_ode`, which is technically not allowed during stages, | ||
| # hence there are no guarantees about the structure part of `v_ode` and `u_ode`. | ||
| # By copying the current split integration values, we make sure that it's correct. |
| foreach_system(semi_split) do system | ||
| # Construct string for the interactions timer. | ||
| # Avoid allocations from string construction when no timers are used. | ||
| # TODO do we need to disable timers in split integration? |

This PR
In step-level coupling, the fluid is advanced a full step before the structure is advanced a full step. This reduces the stability of the main time integration, reducing the maximum stable time step by a factor of 2 (in my tests with Carpenter-Kennedy).
Stage-level coupling calls the sub-integration in every fluid RK stage. The advantage is that the stability properties of the time integrator are preserved. In the current version, this does not have a significant performance impact and is therefore the default (edit: it does not work for some RK schemes with non-monotonic stage times and is therefore not the default). Only for small ratios (like 2-5x smaller time step for the structure), the step-level coupling might be more efficient. For details on how this is implemented, check out my comment below. The implemented version is the one without "restart", and "predict" is enabled by default but can be disabled via kwarg.
For 1.9e-3, I get "instability detected" with stage-level coupling. The maximum stable time step of 1.8e-3 with stage-level coupling is the same as when making the ball a moving solid wall boundary (non-elastic).
Note that we cannot do this with a CFL because the
StepsizeCallbackhas an upper limit of 1.2e-3 due to #1048.Note that something is still not right, as the larger time step makes the ball fall deeper with stage-level coupling. (Edit: This is fixed with "predict" below.)