Monthly updating of BCs from file #90

LenkaNovak · 2022-07-13T08:06:31Z

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: Simple I / O pipeline #74
Design: Time Interpolator Design #88

Dependent PRs

no not merge before

Coupling loop performance

the macro is comparable to using functions (though @time larger variability). The pros and cons for this method are here, 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

review fixes format

93: Interface changes pulled from #90 r=LenkaNovak a=LenkaNovak # 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 - [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>

jiahe23 · 2022-07-21T15:04:42Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

@@ -120,19 +129,20 @@ end

 # init coupler
 coupler_field_names = (:T_S, :z0m_S, :z0b_S, :ρ_sfc, :q_sfc, :albedo, :F_A, :F_E, :F_R, :P_liq)
-coupler_fields =
+coupler_fileds =


jiahe23 · 2022-07-21T15:04:51Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

    dates,
    boundary_space,
    FT,
    mask,
-    coupler_fields,
+    coupler_fileds,


Ahh, I missed this guy during the update! Thanks for catching it! 🐛

kmdeck · 2022-07-21T17:02:58Z

experiments/AMIP/moist_mpi_earth/coupler_utils/bcfile_reader.jl

+
+end
+
+no_scalling(x) = swap_space!(x, axes(mask)) # TODO: mask should not be global


no_scaling?

maybe pass in mask as an argument, then it's no longer a global?

I actually moved this to the bcfile_reader.jl, where it's easy to obtain the mask. :)

kmdeck · 2022-07-21T17:04:58Z

experiments/AMIP/moist_mpi_earth/coupler_utils/calendar_timer.jl

 Convert from String ("YYYYMMDD") to Date format  
 """
 strdate_to_datetime(strdate::String) =
-    Dates.Date(parse(Int, strdate[1:4]), parse(Int, strdate[5:6]), parse(Int, strdate[7:8])) # required by the official AMIP input files
+    Dates.DateTime(parse(Int, strdate[1:4]), parse(Int, strdate[5:6]), parse(Int, strdate[7:8])) # required by the official AMIP input files


is this hardcoded to a specific format? should we concerned that that could change?

If we use Dates, then Date() and DateTime() are the default formats, so I would have thought that this is quite safe.

kmdeck · 2022-07-21T17:05:53Z

experiments/AMIP/moist_mpi_earth/slab_ice/slab_init.jl

@@ -60,3 +60,6 @@ function ice_init(::Type{FT}; tspan, saveat, dt, space, ice_mask, stepper = Eule
 end

 get_ice_mask(h_ice, FT) = h_ice > FT(0) ? FT(1) : FT(0)
+
+# file-specific


could we add a doc string explaining what this is doing?

pass in mask so that it is not global?

pass in FT also

kmdeck · 2022-07-21T17:06:00Z

experiments/AMIP/moist_mpi_earth/slab_ocean/slab_init.jl

@@ -55,3 +55,6 @@ function ocean_init(::Type{FT}; tspan, dt, saveat, space, mask, stepper = Euler(

    SlabSimulation(params, Y, space, integrator)
 end
+
+# file specific


could we add a doc string explaining what this is doing?

pass in mask so that it is not global?

pass in FT also

kmdeck · 2022-07-21T17:06:36Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

        joinpath(coupler_output_dir, "total_energy_bucket.png"),
        joinpath(coupler_output_dir, "total_energy_log_bucket.png"),
    )
 end

-@show coupler_sim.fields.P_liq
+@show cs.fields.P_liq


we can remove :) I had added this in debugging a while back

kmdeck · 2022-07-21T17:59:32Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

+        sst_data,
+        "SST",
+        boundary_space,
+        segment_idx0 = [Int(1309)],


hardcoded 1309, is this ok?

I added a function that finds the closest index of the file date to the user-elected start date.

kmdeck · 2022-07-21T18:03:45Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

+                cs.mode.SIC_info,
+            )
+            SIC = cs.mode.SIC_info.monthly_fields[1]
+            ice_mask = ice_sim.integrator.p.Ya.ice_mask .= get_ice_mask.(SIC .- FT(50), FT)


since 50 is hardcoded, and it appears twice (also on line 113 - maybe make a param we pass in?

Good point! I've included it as a keyword argument in get_ice_mask().

kmdeck · 2022-07-21T18:05:06Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl


-else
-    # ocean
+elseif mode_name == "aquaplanet"


is "aquaplanet" a misleading name at all? Not sure the convention. but this "aquaplanet" has land and ice as well.

I actually removed the ice. Since one of the goals was to have a full aquaplanet, shall we remove the land too? It means land won't be included in the conservation check though. Maybe going forward, we can have another case like simple_earth, which demonstrates the conservation of all slab+Bucket components, also with topography?

yes! I think both are great ideas. We can have a full aquaplanet and a full land planet, and then AMIP?

kmdeck · 2022-07-21T18:06:32Z

Maybe we can get rid of this:

ClimaCoupler.jl/experiments/AMIP/moist_mpi_earth/coupler_driver.jl

Line 78 in 6d5c86c

if land_sim_name == "slab"

since we no longer support slab land?

kmdeck · 2022-07-21T18:10:07Z

I think we should add a buildkite job for the amip mode. Whatever plot you are looking at now to make sure it is doing the right thing should be automatically made as part of CI (otherwise it becomes harder for others to make changes to main, since they wouldnt know what plot to make/check...)..

kmdeck · 2022-07-21T18:11:07Z

experiments/AMIP/moist_mpi_earth/coupler_utils/bcfile_reader.jl

+        )
+    elseif Dates.days(date - all_dates[Int(midmonth_idx)]) > 2 # throw error when there are closer initial indices for the bc file data that matches this date0
+        nearest_idx =
+            argmin(abs.(parse(FT, datetime_to_strdate(date)) .- parse.(FT, datetime_to_strdate.(all_dates[:]))))


global lookup of FT

jb-mackay · 2022-07-21T23:50:53Z

experiments/AMIP/moist_mpi_earth/coupler_utils/general_helper.jl

    Δt::I
    t::F
+    tspan::S


Depending on the type of tspan, it may not be necessary to also store Δt. For example, if tspan = tstart:Δt:tstop then Δt = tspan.step.

That would be a neat solution! :) I was following the notation of ClimaAtmos (where they use tspan to set the ODEProblem), and I believe Kat did the same for the Bucket. Maybe to be consistent for now, we can suggest this as part of the holistic interface revamp when we explore Dennis's design?

jb-mackay · 2022-07-21T23:58:20Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

    NamedTuple{coupler_field_names}(ntuple(i -> ClimaCore.Fields.zeros(boundary_space), length(coupler_field_names)))
 model_sims = (atm = atmos_sim, ice = ice_sim, lnd = land_sim, ocn = ocean_sim)
 dates = (; date = [date], date0 = [date0], date1 = [date1])

-coupler_sim = CouplerSimulation(


I think using coupler_sim is clearer!

I agree. The timing macro would break though with cs = coupler_sim within the timing loop (i.e. cs needs to be in the global scope). Because of this behavior, we will need to revisit using this macro vs just functions, but removing it now seems like a major rework, so I put this on the TODO list after the AMIP showcase.

jb-mackay · 2022-07-22T00:03:32Z

experiments/AMIP/moist_mpi_earth/coupler_driver.jl

+    atmos_sim = cs.model_sims.atm
+    land_sim = cs.model_sims.lnd
+    ocean_sim = cs.model_sims.ocn
+    ice_sim = cs.model_sims.ice
+    Δt_cpl = cs.Δt
+    tspan = cs.tspan


Maybe something like this? Would need to do some renaming

Suggested change

atmos_sim = cs.model_sims.atm

land_sim = cs.model_sims.lnd

ocean_sim = cs.model_sims.ocn

ice_sim = cs.model_sims.ice

Δt_cpl = cs.Δt

tspan = cs.tspan

@unpack model_sims, Δt, tspan = cs

@unpack atm, lnd, ocn = model_sims

jb-mackay · 2022-07-22T00:24:01Z

experiments/AMIP/moist_mpi_earth/coupler_utils/bcfile_reader.jl

+struct BCFileInfo{S, V, R, D, C, O, I}
+    datafile_cgll::S
+    varname::V
+    weightfile::S
+    regrid_space::R
+    all_dates::D
+    segment_idx::Vector{Int}
+    segment_idx0::Vector{Int}
+    monthly_fields::C
+    segment_length::Vector{Int}
+    scaling_function::O # TODO: turn this into a func for scaling + offsets etc
+    interpolate_monthly::I
+end


Please add some docs for the fields of this!

jb-mackay · 2022-07-22T00:31:15Z

experiments/AMIP/moist_mpi_earth/coupler_utils/bcfile_reader.jl

+
+end
+
+no_scalling(x) = swap_space!(x, axes(mask)) # TODO: mask should not be global


Should this be no_scaling?

LenkaNovak · 2022-07-25T05:59:42Z

Is this ok to merge? All issues above should be addressed, and we can iterate on the interface in the upcoming PR(s), after the ClimaAtmos update.

kmdeck · 2022-07-25T15:30:07Z

Is this ok to merge? All issues above should be addressed, and we can iterate on the interface in the upcoming PR(s), after the ClimaAtmos update.

sure! After the ClimaAtmos update, we can also add in buildkite runs for the AMIP mode.

format init monthly interp running final tests done format refact push/pull+unit tst for update_midmonth_data format fix cons test fix cons check fix interf update updated and tested format rev updated format scope fix fix format

LenkaNovak · 2022-07-25T16:34:12Z

bors r+

bors · 2022-07-25T17:12:39Z

Build succeeded:

105: Daily interpolation from monthly data (v2) r=LenkaNovak a=LenkaNovak # PULL REQUEST - Update from #91 ## Purpose and Content Adds capability to interpolate linearly between two monthly `Fields`. The demo uses prerequisite PRs outlined in #88 to obtain monthly data. ## Benefits and Risks - benefit: last piece for enabling specifying temporarily varying boundary conditions from a file. - risk: performance slow-down - check are in place (see below) ## Linked Issues - SDI: #74 - Design: #88 ## Dependent PRs - no not merge before #87 , #89 and #90 ## Performance of the final solution - time loop only over added infrastructure of #88 : adding 0.24s per simulation day at `dt = 200s` ## 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 added this to To do in Coupler Plans via automation Jul 13, 2022

LenkaNovak self-assigned this Jul 13, 2022

This was referenced Jul 13, 2022

Time Interpolator Design #88

Closed

Daily interpolation from monthly data #91

Closed

LenkaNovak requested a review from jiahe23 July 14, 2022 13:59

LenkaNovak force-pushed the monthly-ncread branch from 6a8c10f to 25104e6 Compare July 15, 2022 03:00

LenkaNovak marked this pull request as ready for review July 15, 2022 04:36

LenkaNovak force-pushed the monthly-ncread branch from 724ac50 to 26483eb Compare July 15, 2022 05:04

LenkaNovak added a commit that referenced this pull request Jul 18, 2022

interface changes pulled from #90

fdbdbc1

LenkaNovak mentioned this pull request Jul 18, 2022

Interface changes pulled from #90 #93

Merged

10 tasks

LenkaNovak added a commit that referenced this pull request Jul 19, 2022

interface changes pulled from #90

3146a58

review fixes format

LenkaNovak force-pushed the monthly-ncread branch from a1eec8b to 6d5c86c Compare July 20, 2022 19:59

LenkaNovak requested review from kmdeck, jb-mackay and sriharshakandala July 20, 2022 22:08

jiahe23 reviewed Jul 21, 2022

View reviewed changes

kmdeck reviewed Jul 21, 2022

View reviewed changes

jb-mackay reviewed Jul 21, 2022

View reviewed changes

jb-mackay reviewed Jul 22, 2022

View reviewed changes

kmdeck approved these changes Jul 25, 2022

View reviewed changes

init regrid restructure

b221bf0

format init monthly interp running final tests done format refact push/pull+unit tst for update_midmonth_data format fix cons test fix cons check fix interf update updated and tested format rev updated format scope fix fix format

LenkaNovak force-pushed the monthly-ncread branch from 22fa8a2 to b221bf0 Compare July 25, 2022 16:16

bors bot merged commit 62eb27b into main Jul 25, 2022

bors bot deleted the monthly-ncread branch July 25, 2022 17:12

LenkaNovak mentioned this pull request Jul 28, 2022

Daily interpolation from monthly data (v2) #105

Merged

10 tasks

LenkaNovak moved this from To do to Done in Coupler Plans Aug 8, 2022


		end

		no_scalling(x) = swap_space!(x, axes(mask)) # TODO: mask should not be global

Monthly updating of BCs from file #90

Monthly updating of BCs from file #90

Conversation

LenkaNovak commented Jul 13, 2022 • edited Loading

PULL REQUEST

Purpose and Content

Benefits and Risks

Linked Issues

Dependent PRs

Coupling loop performance

PR Checklist

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

LenkaNovak Jul 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmdeck commented Jul 21, 2022

kmdeck commented Jul 21, 2022

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

jb-mackay Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LenkaNovak commented Jul 25, 2022

kmdeck commented Jul 25, 2022

LenkaNovak commented Jul 25, 2022

bors bot commented Jul 25, 2022

LenkaNovak commented Jul 13, 2022 •

edited

Loading

LenkaNovak Jul 22, 2022 •

edited

Loading

jb-mackay Jul 22, 2022 •

edited

Loading