Conservation checker interface #167

LenkaNovak · 2022-11-21T18:21:48Z

Purpose

Move existing energy check to src and extend to water conservation.

Content

ConservationChecker module
move energy checker code to src
extend energy checker to water mass
add unit tests for accumulator
add unit tests for visualization
demonstrate implementation in a new driver (coupler-driver_modular.jl)
add buildkite hook
add documentation

Notes

the energy check plots in Buildkite are identical to those produced using the non-modularized version of the conservation checker, called by coupler_driver.jl.
conservation is not to the required precision. Investigating this is already a TODO in AMIP improvements #135
generalizing the contained functions further requires a re-design of our simulation objects. This will be deferred to the upcoming SDI on optimization and performance.

I have read and checked the items on the review checklist.

szy21

Thanks Lenka! I only quickly went through it because it's a large PR:) I left a few comments. Also, I wonder if it makes sense to add mass conservation as well?

szy21 · 2022-12-08T18:09:01Z

src/ConservationChecker.jl

+    FT = eltype(coupler_sim.surface_masks.land)
+
+    # save radiation source
+    if radiation != nothing


We have this in ClimaAtmos which integrates fluxes at the boundaries. It might be useful when you clean up the code.

That's great! We can update to this upon the next ClimaAtmos release.

szy21 · 2022-12-08T18:12:49Z

docs/src/conservation.md

+
+Only energy and water are currently implemented. 
+
+Note that kinetic energy is not included in the calculation of the global energy, reflecting the formulation on `ClimaAtmos`, which assumes that kinetic energy is negligible in comparison with the moist static energy components. 


I think this is a bit confusing because e.g. when you calculate the total energy of atmosphere using e_tot, it includes kinetic energy. Maybe rephrase it as something like Kinetic energy transfer between atmosphere, land, and ocean is not included?

That was rather misleading, thanks for pointing it out! It was meant to refer to the KE contribution being neglected in all diffusive fluxes in the atmos interior, so this is consistent with these BCs. In any case, since we won't be needing this function (except perhaps for diagnostics?) in the foreseeable, I've decided to remove it for now. :P

szy21 · 2022-12-08T18:21:54Z

src/ConservationChecker.jl

+    evaporation = cs.fields.F_E # kg/m^2/s / layer depth
+    precipitation_l = cs.fields.P_liq
+    precipitation_s = cs.fields.P_snow
+    @. (evaporation + precipitation_l + precipitation_s) * cs.Δt_cpl # in kg 


Suggested change

@. (evaporation + precipitation_l + precipitation_s) * cs.Δt_cpl # in kg

@. (evaporation + precipitation_l + precipitation_s) * cs.Δt_cpl # in kg /m^2

I think that's the unit? I'm not sure though.

see above. Now clarified

szy21 · 2022-12-08T18:26:15Z

src/ConservationChecker.jl

+    end
+
+    # save ocean
+    if ice_sim !== nothing


Suggested change

if ice_sim !== nothing

if ocean_sim !== nothing

?

Good catch! I missed this after the last rebase! 😳

szy21 · 2022-12-08T18:30:23Z

src/ConservationChecker.jl

+    if land_sim !== nothing
+        water_content =
+            @. (land_sim.integrator.u.bucket.σS + land_sim.integrator.u.bucket.W + land_sim.integrator.u.bucket.Ws) # m^3 water / land area / layer height
+        parent(water_content) .= parent(water_content .* surface_masks.land) * FT(1000)


It's a bit hard for me to figure out the unit here (without knowing details about the land model). Maybe add some more comments?

Sure! I can comment more lines. Also, instead of the FT(1000), I'm adding ClimaLSM.LSMP.ρ_cloud_liq which clarifies the units further. The final values are in kg, thanks to ClimaCore's sum function doing the volume integral for us (rather than a simple sum).

juliasloan25 · 2022-12-08T21:53:39Z

artifacts/Artifacts.toml

@@ -0,0 +1,8 @@
+[land_mask]


do we want the artifacts to be checked in to github? I'm not sure it matters too much, just wanted to double check

I would have thought it would be good to keep it for tracking changes in the files (each time the file is replaced with a new one of the same name, the git-tree-sha1 changes). I just found this issue, which may require using Tar.tree_hash to be general to all platforms, but apparently this should have been fixed in Julia 1.6. @charleskawczynski do you have any thoughts? I noticed Atmos doesn't check these in?

Yes, I think the artifacts toml is the intended file to be tracked, so that there's a direct mapping between these hashes and the artifacts used.

We don't in ClimaAtmos, but we probably should.

Also, I haven't seen that issue before, it could be that we somehow need to update ArtifactWrappers to use this

For now, I think checking it in is fine, it at least enforces some reproducibility

Thank you, Charlie!

juliasloan25 · 2022-12-08T21:57:40Z

experiments/AMIP/moist_mpi_earth/coupler_driver_modular.jl

+end
+
+## read in some parsed command line arguments 
+mode_name = parsed_args["mode_name"]


maybe not now, but we could consider reading in parsed_args as a NamedTuple, then using the (; ...) syntax to unpack it more concisely

juliasloan25 · 2022-12-08T22:01:07Z

experiments/AMIP/moist_mpi_earth/coupler_driver_modular.jl

+
+# import coupler utils
+include("coupler_utils/flux_calculator.jl")
+include("coupler_utils/regridder.jl") # update_midmonth_data!


I think we don't need this since the Regridder module should have all the functionality of the regridder util. If we still have the include, we might not be actually using the functions from Regridder

juliasloan25 · 2022-12-09T01:13:50Z

src/ConservationChecker.jl

+
+using ClimaCoupler.Utilities
+using ClimaCore: ClimaCore, Geometry, Meshes, Domains, Topologies, Spaces, Fields, InputOutput
+using ClimaCore.Utilities: half


I think this is redundant since you already have using ClimaCoupler.Utilities

Actually the half needs to be specifically imported, but line 8 can be removed. :)

juliasloan25 · 2022-12-09T01:18:27Z

src/ConservationChecker.jl

+"""
+    check_conservation!(coupler_sim::CoupledSimulation)
+
+itertes over all specified conservation checks.


docstring doesn't match function signature

Good catch!

juliasloan25 · 2022-12-09T01:19:54Z

src/ConservationChecker.jl

+    map(x -> check_conservation!(x, coupler_sim, get_slab_energy, get_land_energy), coupler_sim.conservation_checks)
+
+"""
+        check_conservation!(


docstring doesn't match function signature

juliasloan25 · 2022-12-09T01:26:03Z

src/ConservationChecker.jl

+
+Calculate the kinetic energy diffipation of a simulation. 
+"""
+function ke_dissipation(sim)


could be helpful to annotate the input and return types (at least in the docstring)

This function is now removed (see above).

juliasloan25 · 2022-12-09T01:26:21Z

src/ConservationChecker.jl

+end
+
+"""
+        check_conservation(


docstring doesn't match function signature

juliasloan25 · 2022-12-09T01:31:59Z

src/ConservationChecker.jl

+end
+
+# https://github.com/jheinen/GR.jl/issues/278#issuecomment-587090846
+ENV["GKSwstype"] = "nul"


a short comment explaining why this is here could be helpful, then readers can follow the link for more detail

LenkaNovak · 2022-12-09T12:58:13Z

@szy21 and @juliasloan25 , thank you both for the review! 🚀

LenkaNovak · 2022-12-09T13:01:38Z

We can do, but I'm not sure we're exchanging anything else but water between the models right now, so maybe domain-local conservation checks should be done in the models themselves?

szy21 · 2022-12-09T17:34:58Z

We can do, but I'm not sure we're exchanging anything else but water between the models right now, so maybe domain-local conservation checks should be done in the models themselves?

Yes, I just thought it would be more comprehensive and might be useful for debugging to have all the conservation checks (mass, energy, water) when running the coupled simulation. But you're right there is no mass exchange between atmos and land or ocean, so we can do it in ClimaAtmos.

LenkaNovak · 2022-12-09T17:51:40Z

It's really no problem to add it in here temporarily, if it's useful. But in the long term we should probably have checks for both coupled and standalone runs. I'll put it on the AMIP improvements list and maybe we can reassess in Jan?

tests for all except land_sea_mask all tests pass, land_sea_mask separated before Project.toml cleanup Project.tomls cleaned up cleanup GeneralUtilities, TestHelper modules docs and formatting modularize conservation checker wip passing tests clean + doc doc trigger test docs rebase plots rebase fix dep + buildkite buildkite fix plots dep unavoidable pip fix tests deps redundant func fix fix revs SB Manifest

LenkaNovak · 2022-12-10T01:42:38Z

bors r+

bors · 2022-12-10T03:46:35Z

Build succeeded:

LenkaNovak mentioned this pull request Nov 21, 2022

ClimaCoupler interface #134

Closed

13 tasks

LenkaNovak force-pushed the ln/conservationchecker-interface branch 3 times, most recently from cebcc63 to db64044 Compare December 8, 2022 16:06

LenkaNovak marked this pull request as ready for review December 8, 2022 16:07

LenkaNovak requested a review from juliasloan25 December 8, 2022 16:07

LenkaNovak force-pushed the ln/conservationchecker-interface branch from db64044 to b739663 Compare December 8, 2022 16:33

LenkaNovak changed the title ~~Conservation checker interface (WIP)~~ Conservation checker interface Dec 8, 2022

LenkaNovak requested a review from szy21 December 8, 2022 16:34

szy21 reviewed Dec 8, 2022

View reviewed changes

szy21 approved these changes Dec 8, 2022

View reviewed changes

juliasloan25 approved these changes Dec 9, 2022

View reviewed changes

LenkaNovak force-pushed the ln/conservationchecker-interface branch from b739663 to 223542b Compare December 9, 2022 17:46

LenkaNovak force-pushed the ln/conservationchecker-interface branch from 223542b to 5055449 Compare December 9, 2022 23:31

bors bot merged commit b230014 into main Dec 10, 2022

bors bot deleted the ln/conservationchecker-interface branch December 10, 2022 03:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conservation checker interface #167

Conservation checker interface #167

LenkaNovak commented Nov 21, 2022 •

edited

Loading

szy21 left a comment

szy21 Dec 8, 2022

LenkaNovak Dec 9, 2022

szy21 Dec 8, 2022

LenkaNovak Dec 9, 2022

szy21 Dec 8, 2022

LenkaNovak Dec 9, 2022

szy21 Dec 8, 2022

LenkaNovak Dec 9, 2022

szy21 Dec 8, 2022

LenkaNovak Dec 9, 2022

juliasloan25 Dec 8, 2022

LenkaNovak Dec 9, 2022

charleskawczynski Dec 9, 2022

charleskawczynski Dec 9, 2022

LenkaNovak Dec 9, 2022

juliasloan25 Dec 8, 2022

juliasloan25 Dec 8, 2022

juliasloan25 Dec 9, 2022

LenkaNovak Dec 9, 2022 •

edited

Loading

juliasloan25 Dec 9, 2022

LenkaNovak Dec 9, 2022

juliasloan25 Dec 9, 2022

juliasloan25 Dec 9, 2022

LenkaNovak Dec 9, 2022 •

edited

Loading

juliasloan25 Dec 9, 2022

juliasloan25 Dec 9, 2022

LenkaNovak Dec 9, 2022

LenkaNovak commented Dec 9, 2022

LenkaNovak commented Dec 9, 2022 •

edited

Loading

szy21 commented Dec 9, 2022

LenkaNovak commented Dec 9, 2022 •

edited

Loading

LenkaNovak commented Dec 10, 2022

bors bot commented Dec 10, 2022


		Only energy and water are currently implemented.

		Note that kinetic energy is not included in the calculation of the global energy, reflecting the formulation on `ClimaAtmos`, which assumes that kinetic energy is negligible in comparison with the moist static energy components.

	@. (evaporation + precipitation_l + precipitation_s) * cs.Δt_cpl # in kg
	@. (evaporation + precipitation_l + precipitation_s) * cs.Δt_cpl # in kg /m^2

Conservation checker interface #167

Conservation checker interface #167

Conversation

LenkaNovak commented Nov 21, 2022 • edited Loading

Purpose

Content

Notes

szy21 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LenkaNovak Dec 9, 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

LenkaNovak Dec 9, 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

LenkaNovak commented Dec 9, 2022

LenkaNovak commented Dec 9, 2022 • edited Loading

szy21 commented Dec 9, 2022

LenkaNovak commented Dec 9, 2022 • edited Loading

LenkaNovak commented Dec 10, 2022

bors bot commented Dec 10, 2022

LenkaNovak commented Nov 21, 2022 •

edited

Loading

LenkaNovak Dec 9, 2022 •

edited

Loading

LenkaNovak Dec 9, 2022 •

edited

Loading

LenkaNovak commented Dec 9, 2022 •

edited

Loading

LenkaNovak commented Dec 9, 2022 •

edited

Loading