Interface changes pulled from #90 #93

LenkaNovak · 2022-07-18T14:53:12Z

PULL REQUEST

Purpose and Content

This PR introduces interface changes which are used in PR #90 . They are split from the main PR to make reviewing of the functionality easier. The changes here include:

reducing arguments in push! and pull functions by introducing a CouplerSimulation struct
unifying names of, and information contained within, model sims. This reduces the number of if.. else.. statements when running with prescribed or modeled SSTs.
include SST and SIC in their respective sims

While these interface changes are not part of the final coupler interface design, they facilitate the development and running of the models from the coupler in the intermediate term, and will be used in the first AMIP showcase.

Benefits and Risks

b: less code, more structured
r: interface changes are not the priority (only minimal changes introduced here)

Linked Issues

see #90

PR Checklist

kmdeck · 2022-07-18T16:23:35Z

hey! Not sure what happened, but the buildkite plots no longer are saving correctly - they did as of this PR:
#81
https://buildkite.com/clima/climacoupler-ci/builds/119#0181df9e-b5b3-4949-b7e0-63643b4e6f41

The file paths look fine/unchanged, so maybe more likely that the plot is not being generated?
https://buildkite.com/clima/climacoupler-ci/builds/159#018211c3-5376-43b1-bd38-b5ae77354b28/207-209

kmdeck · 2022-07-18T16:32:50Z

experiments/AMIP/moist_mpi_earth/push_pull.jl

-    FT = eltype(F_A)
-    @. slab_sim.integrator.p.bucket.ρ_sfc = ρ_sfc
-    @. slab_sim.integrator.p.bucket.SHF = F_A
+function land_pull!(cs)


How do we execute the different methods of land_pull? prior to this, they dispatched on the type of land model.
I think we need land_pull!(land_sim::BucketSimulation, cs) and land_pull!(land_sim::SlabSimulation, cs). in general we will have to dispatch on the type of model, so this format might be nice for all (land, ocean, ice)?

@kmdeck's suggestion works for sure, or you can do an if block. Not terribly important imo; when the Simulations + CouplerState are introduced at each level, the push/pull operations can more easily dispatch on simulation and model type.

Yeah, this was just a temporary solution for this AMIP case, until we deploy Ben's Simulations interface, which will def allow this multiple dispatch - I should have been clearer in the PR description above. Are you guys happy with this for now, since land_pull.jl will be scrapped eventually anyway?

Hm, not sure I understand. If we leave as is, what method of land_pull! will be executed if the land model is a bucket, vs the land modeling being a slab? Alternatively, we can scrap the slab being an option for the land?

(having a stand-in is fine, but my point was more to make sure the temporary soln has the capabilities we want to support)

No, it is a good point! From the AMIP roadmap perspective, I don't think we need to support a thermal-slab land, but if you think it would be valuable to have it here, then I can add it. Lmk when you're free next, maybe we can chat on Zoom and decide 😎 .

free anytime this morning! I'm fine losing the thermal slab land in this experiment directory though 🌤️

kmdeck · 2022-07-18T16:33:33Z

experiments/AMIP/moist_mpi_earth/push_pull.jl

+function land_pull!(cs)
+    slab_sim = cs.model_sims.lnd
+    csf = cs.fields
+    FT = eltype(csf.F_A)


csf has FT stored directly.

jb-mackay · 2022-07-18T16:36:36Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

-    ocean_pull!(slab_ocean_sim, F_A, F_R)
-    reinit!(slab_ocean_sim.integrator)
-end
+    coupler_fileds,


Suggested change

coupler_fileds,

coupler_fields,

Needs to be changed in a few other spots, too!

kmdeck · 2022-07-18T17:29:14Z

experiments/AMIP/moist_mpi_earth/push_pull.jl

+    land_mask = cs.mask
+
+    # mask of all models (1 = land, 0 = ocean, -2 = sea ice)
+    univ_mask = parent(land_mask) .- parent(ice_mask .* FT(2))


get FT from coupler sim object, right now it is looking up in global here I think

kmdeck · 2022-07-18T17:30:37Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

 end

+prescribed_sst = true


should be read in from command line - this overwrites it. I think this is why the buildkite plot isnt being made.

Also, it seems like prescribed_sst now flips between aquaplanet and AMIP. Should we be passing that in, instead? i.e. pass in a CLI called mode which is either amip or aquaplanet, rather than pass in prescribed_sst? Because prescribed_sst also affects what ice is doing. maybe a bit confusing as is.

Oops! This line shouldn't be there (it was leftover from a test - nice catch!). As for changing this flag to prescribed_sst -> mode (or something like that), that can be done. :)

kmdeck · 2022-07-18T17:35:59Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

 walltime = @elapsed for t in ((tspan[1] + Δt_cpl):Δt_cpl:tspan[end])
+    cs = coupler_sim


is this needed? why not use coupler_sim directly?

it's needed in the next PR - I didn't want to change everything in the stepping loop only to change it all back in the next PR... 😳

kmdeck · 2022-07-18T17:36:41Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

    end

    ## Slab ice
-    ice_pull!(slab_ice_sim, F_A, F_R)
-    step!(slab_ice_sim.integrator, t - slab_ice_sim.integrator.t, true)
+    cs.mode.prescribed_sst ? ice_pull!(cs) : nothing


should these be in a single if statement? otherwise we check multiple times.

jb-mackay

Largely looks great @LenkaNovak - this is a good intermediate update of the interface and its clear how it will converge with the sea breeze interface work. Nice to see! Some small changes to fix before merging that I noted.

jb-mackay · 2022-07-18T20:29:26Z

It might be possible to remove some of the swap_space! functions using the ClimaCore broadcasting mechanics. See #79 for how to enable that. This could be a follow up PR.

LenkaNovak · 2022-07-18T23:14:28Z

hey! Not sure what happened, but the buildkite plots no longer are saving correctly - they did as of this PR: #81 https://buildkite.com/clima/climacoupler-ci/builds/119#0181df9e-b5b3-4949-b7e0-63643b4e6f41

The file paths look fine/unchanged, so maybe more likely that the plot is not being generated? https://buildkite.com/clima/climacoupler-ci/builds/159#018211c3-5376-43b1-bd38-b5ae77354b28/207-209

Hmm, this is really odd. Looks like this was already an issue in #89, but we hadn't picked it up. I'll investigate further.

review fixes format

kmdeck

looks great Lenka! Nice work!

LenkaNovak · 2022-07-19T20:52:23Z

bors r+

bors · 2022-07-19T21:29:29Z

Build succeeded:

90: Monthly updating of BCs from file r=LenkaNovak a=LenkaNovak # PULL REQUEST ## Purpose and Content This PR adds functionality to collect BC field information in a struct `..._info`, which is then updated each month with a new file read of the regridded data. In the next PR `..._info` will contain two Fields read from a file, which will be used for the daily interpolation. ## Benefits and Risks - benefit: prerequisite for the monthly to daily interpolation - risk: changes the way the BCs are read, but most of it is abstracted ## Linked Issues - SDI: #74 - Design: #88 ## Dependent PRs - no not merge before - [x] #87 - [x] #89 - [x] #93 ## Coupling loop performance - the macro is comparable to using functions (though `@time` larger variability). The pros and cons for this method are [here](#95), are will be revisited post AMIP (and well as including the unit test in the same file). - in either case the monthly update does not cause noticeable performance change from the previous commit for the full AMIP setup. ## PR Checklist - [x] This PR has a corresponding issue OR is linked to an SDI. - [x] I have followed CliMA's codebase [contribution](https://clima.github.io/ClimateMachine.jl/latest/Contributing/) and [style](https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/) guidelines OR N/A. - [x] I have followed CliMA's [documentation policy](https://github.com/CliMA/policies/wiki/Documentation-Policy). - [x] I have checked all issues and PRs and I certify that this PR does not duplicate an open PR. - [x] I linted my code on my local machine prior to submission OR N/A. - [x] Unit tests are included OR N/A. - [x] Code used in an integration test OR N/A. - [x] All tests ran successfully on my local machine OR N/A. - [x] All classes, modules, and function contain docstrings OR N/A. - [x] Documentation has been added/updated OR N/A. Co-authored-by: LenkaNovak <lenka@caltech.edu>

LenkaNovak marked this pull request as ready for review July 18, 2022 14:54

LenkaNovak requested review from kmdeck and jb-mackay July 18, 2022 14:54

kmdeck reviewed Jul 18, 2022

View reviewed changes

jb-mackay reviewed Jul 18, 2022

View reviewed changes

kmdeck reviewed Jul 18, 2022

View reviewed changes

jb-mackay approved these changes Jul 18, 2022

View reviewed changes

LenkaNovak mentioned this pull request Jul 18, 2022

Interface issues in experiment drivers #92

Closed

interface changes pulled from #90

3146a58

review fixes format

LenkaNovak force-pushed the ln/push-pull-args branch from 8961704 to 3146a58 Compare July 19, 2022 01:39

kmdeck approved these changes Jul 19, 2022

View reviewed changes

bors bot merged commit 6bd105e into main Jul 19, 2022

bors bot deleted the ln/push-pull-args branch July 19, 2022 21:29

LenkaNovak mentioned this pull request Jul 20, 2022

Monthly updating of BCs from file #90

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interface changes pulled from #90 #93

Interface changes pulled from #90 #93

LenkaNovak commented Jul 18, 2022 •

edited

Loading

kmdeck commented Jul 18, 2022 •

edited

Loading

kmdeck Jul 18, 2022

jb-mackay Jul 18, 2022

LenkaNovak Jul 18, 2022

kmdeck Jul 18, 2022

kmdeck Jul 18, 2022

LenkaNovak Jul 19, 2022

kmdeck Jul 19, 2022

kmdeck Jul 18, 2022

jb-mackay Jul 18, 2022

jb-mackay Jul 18, 2022

kmdeck Jul 18, 2022

kmdeck Jul 18, 2022

kmdeck Jul 18, 2022

LenkaNovak Jul 18, 2022

kmdeck Jul 18, 2022

LenkaNovak Jul 18, 2022

kmdeck Jul 18, 2022 •

edited

Loading

jb-mackay left a comment

jb-mackay commented Jul 18, 2022

LenkaNovak commented Jul 18, 2022 •

edited

Loading

kmdeck left a comment

LenkaNovak commented Jul 19, 2022

bors bot commented Jul 19, 2022

		walltime = @elapsed for t in ((tspan[1] + Δt_cpl):Δt_cpl:tspan[end])
		cs = coupler_sim

Interface changes pulled from #90 #93

Interface changes pulled from #90 #93

Conversation

LenkaNovak commented Jul 18, 2022 • edited Loading

PULL REQUEST

Purpose and Content

Benefits and Risks

Linked Issues

PR Checklist

kmdeck commented Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmdeck Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

jb-mackay left a comment

Choose a reason for hiding this comment

jb-mackay commented Jul 18, 2022

LenkaNovak commented Jul 18, 2022 • edited Loading

kmdeck left a comment

Choose a reason for hiding this comment

LenkaNovak commented Jul 19, 2022

bors bot commented Jul 19, 2022

LenkaNovak commented Jul 18, 2022 •

edited

Loading

kmdeck commented Jul 18, 2022 •

edited

Loading

kmdeck Jul 18, 2022 •

edited

Loading

LenkaNovak commented Jul 18, 2022 •

edited

Loading