Conversation
tomchor
changed the title
PerturbationAdvectionOpenBoundaryCondition
| schemes = [ | ||
| PerturbationAdvection(inflow_timescale=2, outflow_timescale=1), | ||
| ] | ||
|
|
||
| for timestepper in (:QuasiAdamsBashforth2, :RungeKutta3), scheme in schemes | ||
| scheme_name = replace(string(typeof(scheme)), "." => "_") |
There was a problem hiding this comment.
Included a loop over schemes in anticipation of @simone-silvestri 's upcoming implementation of other radiation schemes. That way it'll make it easier to add them here for testing.
tomchor
requested review from
ali-ramadhan and
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR addresses checkpoint pickup failures for open-domain simulations that use OpenBoundaryConditions with non-trivial schemes (eg PerturbationAdvection), by extending checkpoint restore support and adding a regression test.
Changes:
- Add a
restore_prognostic_state!method to handle restoring numeric values from checkpoints. - Add a new checkpointing regression test that exercises
OpenBoundaryCondition(...; scheme=...)and validatesboundary_mass_fluxesplus continued execution after pickup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/OutputWriters/checkpointer.jl |
Adds numeric restore dispatch to prevent MethodError during checkpoint restoration. |
test/test_checkpointer.jl |
Adds a regression test for checkpointing + pickup with OpenBoundaryCondition schemes (eg PerturbationAdvection). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
tomchor
changed the title
OpenBoundaryConditionOpenBoundaryCondition
src/OutputWriters/checkpointer.jl
Outdated
| return restore_prognostic_state!(restored_parent, from_parent) | ||
| end | ||
|
|
||
| restore_prognostic_state!(restored::Number, from::Number) = convert(typeof(restored), from) |
There was a problem hiding this comment.
Just to check --- Number is immutable, so somehow this is having an effect because the returned value from restore_prognostic_state! is used. In other cases, as I understand, restore_prognostic_state! has no return (eg it mutates something in place).
There was a problem hiding this comment.
Right. I think restored is already the same as from in all situations given that it's built directly from the grid when building NonhydrostaticModel (which at this point I think has been checked to match). The error appeared only because such a method was called but wasn't defined, but maybe it's unnecessary. I'll leave the method there since it's cleaner logic (and just in case I'm wrong).
I did change the function to explicitly modify restored, that way it's more in line with the other methods and with what is expected from its name (i.e. the ! at the end). I'll merge when tests pass.
* glw/prescribed-free-surface: (0.105.3) Fix interpolation of terms in chain horizontal derivatives for MutableVerticalDiscretization (CliMA#5339) Fix ContinuousForcing field index mismatch in HydrostaticFreeSurfaceModel (CliMA#5370) Add dependency guardrail to Common Pitfalls (CliMA#5371) Fix typo SKR3 -> SRK3 (CliMA#5360) Add new reference to Zheng et al. (2025) in index (CliMA#5364) Add 9 missing papers to the reference list (CliMA#5363) Update papers section in index.md (CliMA#5362) Restructure AGENTS.md into multi-file agent configuration (CliMA#5350) (0.105.2) Fix `with_halo` for a `TripolarGrid` (CliMA#5353) (0.105.2) Fix and test checkpointing with open-domain simulations with a radiation `OpenBoundaryCondition` (CliMA#5347)
Closes #5346
Making it a draft for now because I wanna make sure we catch the relevant errors before fixing it.