Skip to content

Conversation

@navidcy
Copy link
Member

@navidcy navidcy commented Mar 20, 2025

This PR enhances tick!(clock, Δt). Also defines method for set_clock!(clock, new_clock).

Historically this PR started by implementing some changes needed for the implementation of a Checkpointer for the OceanSeaIceModel at CliMA/ClimaOcean.jl#401. But in the process we bumped onto things related to clock and time stepping.

Main change is that properties is no longer a field of the Checkpointer but rather it's inferred from the model. This way we have different properties for the different model components of a coupled model in ClimaOcean.

Also the PR adds tests for the Checkpointer that use the SplitExplicitFreeSurface (two formulations via constant substeps and given cfl).

@navidcy navidcy requested review from glwagner and simone-silvestri and removed request for simone-silvestri March 20, 2025 08:49
@navidcy
Copy link
Member Author

navidcy commented Mar 20, 2025

@glwagner the CI machine seems out of space?

ERROR: Unable to automatically download/install artifact 'CUDA_Runtime' from sources listed in '/storage5/buildkite-agent/.julia-21600/packages/CUDA_Runtime_jll/iU7vK/Artifacts.toml'.
Sources attempted:
- https://pkg.julialang.org/artifact/59c3479d656c293c66e1a7a26fcb5d860dbe6235
    Error: SystemError: flush: No space left on device
- https://github.com/JuliaBinaryWrappers/CUDA_Runtime_jll.jl/releases/download/CUDA_Runtime-v0.16.1+0/CUDA_Runtime.v0.16.1.x86_64-linux-gnu-cuda+12.8.tar.gz
    Error: SystemError: flush: No space left on device

@navidcy navidcy changed the title Checkpointer updates (0.96.1) Checkpointer updates Mar 20, 2025
@navidcy navidcy changed the title (0.96.1) Checkpointer updates (0.96.2) Checkpointer updates Mar 22, 2025
@tomchor
Copy link
Collaborator

tomchor commented Mar 22, 2025

Do the changes here have any impact on #3485?

i.e. will it make it easier or harder to make Checkpointer play with time-averaged fields?

@glwagner
Copy link
Member

We should probably at the least address the problem of model vs simulation checkpointing here

@navidcy
Copy link
Member Author

navidcy commented Mar 22, 2025

@tomchor it shouldn't affect the time-averaging issue in either way I think.

@navidcy
Copy link
Member Author

navidcy commented Mar 22, 2025

@glwagner, @tomchor so I see in #3485 the discussion/proposal is to have write_output!(sim::Simulation, ...) instead of write_output!(model::AbstractModel, ...).

We can do that here. But then it means that every time somebody defines a new model (and I'm thinking of ClimaOcean.jl and beyond) then we need to define a new method for write_output!(sim::Simulation{MyNewModel}, ...) instead of write_output!(model::MyNewModel, ...), right?

@tomchor
Copy link
Collaborator

tomchor commented Mar 25, 2025

@glwagner, @tomchor so I see in #3485 the discussion/proposal is to have write_output!(sim::Simulation, ...) instead of write_output!(model::AbstractModel, ...).

We can do that here. But then it means that every time somebody defines a new model (and I'm thinking of ClimaOcean.jl and beyond) then we need to define a new method for write_output!(sim::Simulation{MyNewModel}, ...) instead of write_output!(model::MyNewModel, ...), right?

I think so, but wouldn't a fallback like write_output!(simulation) = write_output!(simulation.model) take care of this?

@glwagner
Copy link
Member

@glwagner, @tomchor so I see in #3485 the discussion/proposal is to have write_output!(sim::Simulation, ...) instead of write_output!(model::AbstractModel, ...).
We can do that here. But then it means that every time somebody defines a new model (and I'm thinking of ClimaOcean.jl and beyond) then we need to define a new method for write_output!(sim::Simulation{MyNewModel}, ...) instead of write_output!(model::MyNewModel, ...), right?

I think so, but wouldn't a fallback like write_output!(simulation) = write_output!(simulation.model) take care of this?

Yeah no I think we can have a pretty small PR that simply changes the meaning of write_output!. A few lines are all that's needed:

write_output!(writer, sim::Simulation) = write_output!(writer, sim.model)

and then I think 2-3 places in Simulations where we call write_output!.

I'd prefer if that was self-contained, we should avoid the temptation to make PRs bigger than needed.

The trickier part (but not the hardest thing ever in the world) is to design an interface for checkpointing callbacks, diagnostics, and output writers.

@navidcy
Copy link
Member Author

navidcy commented Jun 26, 2025

This PR includes few useful updates/refactors for Clock and I'm quite tempted to remove the Checkpointer things, merge and continue with Checkpointer in a new PR. What do you think?

@navidcy navidcy changed the title (0.96.2) Clock and Checkpointer updates Clock and Checkpointer updates Jun 26, 2025
@tomchor
Copy link
Collaborator

tomchor commented Jun 26, 2025

This PR includes few useful updates/refactors for Clock and I'm quite tempted to remove the Checkpointer things, merge and continue with Checkpointer in a new PR. What do you think?

I'm all for simple PRs!

@navidcy navidcy mentioned this pull request Jul 6, 2025
@navidcy
Copy link
Member Author

navidcy commented Jul 6, 2025

#4637 attempts to separate the Clock enhancements

@navidcy navidcy changed the title Clock and Checkpointer updates Checkpointer updates Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

What does align_time_step = true does in Simulation?

4 participants