Skip to content

Implementing checkpointer for coupled model #401

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.

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

Open
wants to merge 67 commits into
base: main
Choose a base branch
from

Conversation

taimoorsohail
Copy link
Collaborator

@taimoorsohail taimoorsohail commented Mar 11, 2025

This PR supersedes #381 adding a new run_coupled! function that replaces the run! function in Oceananigans. adds new methods so that checkpointing works for coupled ocean-sea ice simulations with PrescribedAtmosphere.

with @navidcy

@glwagner
Copy link
Member

I don't agree with the design that creates a new function called run_coupled!. However, one could extend run! for a Simulation of OceanSeaIceModel. I'm not sure if there is a reason to keep both versions of run!.

@navidcy
Copy link
Member

navidcy commented Mar 11, 2025

I don't agree with the design that creates a new function called run_coupled!. However, one could extend run! for a Simulation of OceanSeaIceModel. I'm not sure if there is a reason to keep both versions of run!.

Yes, we also agree. We don't like that either!
We were playing around with extending run! but couldn't figure out a way to differentiate the methods...

Somehow this:

function run!(simulation::Simulation{<:OceanSeaIceSimulation}`; pickup)
    if pickup
        ... do extra things to pick up a coupled model
    end

    run!(simulation) # Oceananigans.Simulations.run!
    return nothing
end

wasn't working for us yesterday.

Don't worry too much atm, it's mostly playing around. When we settle or if we get stuck we can ping you.

Copy link

codecov bot commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 0% with 39 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (9efec12) to head (9f9ca89).

Files with missing lines Patch % Lines
src/OceanSeaIceModels/ocean_sea_ice_model.jl 0.00% 21 Missing ⚠️
src/OceanSeaIceModels/PrescribedAtmospheres.jl 0.00% 17 Missing ⚠️
src/ClimaOcean.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #401   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         40      40           
  Lines       2300    2329   +29     
=====================================
- Misses      2300    2329   +29     

☔ 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.

@taimoorsohail
Copy link
Collaborator Author

Linking issue #404 to this PR

@glwagner
Copy link
Member

glwagner commented Mar 12, 2025

I don't agree with the design that creates a new function called run_coupled!. However, one could extend run! for a Simulation of OceanSeaIceModel. I'm not sure if there is a reason to keep both versions of run!.

Yes, we also agree. We don't like that either! We were playing around with extending run! but couldn't figure out a way to differentiate the methods...

Somehow this:

function run!(simulation::Simulation{<:OceanSeaIceSimulation}`; pickup)
    if pickup
        ... do extra things to pick up a coupled model
    end

    run!(simulation) # Oceananigans.Simulations.run!
    return nothing
end

wasn't working for us yesterday.

Don't worry too much atm, it's mostly playing around. When we settle or if we get stuck we can ping you.

To implement this, you need to first work on a PR in Oceananigans that exposes an interface for "picking up a simulation", which has a default implementation in Oceananigans, but which can be specialized for the type of Simulation. Then you can specialize for a simulation of OceanSeaIceModel here.

@navidcy
Copy link
Member

navidcy commented Mar 13, 2025

After 9ab0df1 I can run the checkpointer_mwe.jl with Oceananigans 0.95.26 (not the branch from CliMA/Oceananigans.jl#4216)!

@navidcy
Copy link
Member

navidcy commented Mar 13, 2025

@glwagner can you review? if you are happy we can convert the checkpointer_mwe.jl into a test and we are good to go I think

@navidcy navidcy requested a review from glwagner March 13, 2025 22:39
@navidcy navidcy added the enhancement New feature or request label Mar 13, 2025
@navidcy
Copy link
Member

navidcy commented Mar 19, 2025

Just note that I’m refactoring this. It’s almost there. But haven’t pushed yet.

@navidcy
Copy link
Member

navidcy commented Mar 20, 2025

x-ref CliMA/Oceananigans.jl#4250

@navidcy
Copy link
Member

navidcy commented Apr 30, 2025

I plan to work on this early next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request output 💾
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkpointer not supported for coupled ClimaOcean workflows Proper support for run!(simulation, pickup=true) with OceanSeaIceModel
3 participants