Tr/isolate run amip postprocessing #1120

imreddyTeja · 2024-12-12T18:14:04Z

Purpose

To-do

Content

This PR adds a struct for each different mode_name.
A struct is returned by get_cupler_args as mode_type (suggest a better name for the var).
This is used everywhere mode_name was. The structs do have hierarchy,
but the Abstract types should be given better names.

It also adds a new file,
experiments/ClimaEarth/user_io/postprocessing.jl, which contains
the postprocess_sim function. This function copies the if statements
previously at the end of run_amip.jl, but it uses dispatch for the simulation
mode. The postprocessing if statements are now moved into the functions.

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

imreddyTeja · 2024-12-12T21:34:42Z

experiments/ClimaEarth/user_io/arg_parsing.jl

@@ -1,5 +1,21 @@
 import YAML

+abstract type AbstractModeType end
+abstract type AbstractSlabPlanetModeType <: AbstractModeType end
+abstract type AbstractSlabPlanetModeSubType1 <: AbstractSlabPlanetModeType end


This needs a better name. It is the group of modes that is used for model setup here

Or it could just be removed

In the model setup, we could dispatch off of AbstractSlabPlanetModeType, and have a special case just for SlabPlanetEisenman (removing the need for AbstractSlabPlanetModeSubType1

juliasloan25 · 2024-12-16T18:01:50Z

experiments/ClimaEarth/user_io/arg_parsing.jl

@@ -1,5 +1,21 @@
 import YAML

+abstract type AbstractModeType end


We could change ModeType -> SimMode here and throughout (e.g. mode_type would become sim_mode too). What do you think?

SimulationMode

move these to Interfacer module

juliasloan25 · 2024-12-16T18:04:39Z

experiments/ClimaEarth/user_io/postprocessing.jl

+"""
+    common_postprocessing(cs, postprocessing_vars)
+
+Perfrom postprocessing common to all simulation types.


Suggested change

Perfrom postprocessing common to all simulation types.

Perform postprocessing common to all simulation types.

juliasloan25 · 2024-12-16T18:05:15Z

experiments/ClimaEarth/user_io/arg_parsing.jl

+abstract type AbstractModeType end
+abstract type AbstractSlabPlanetModeType <: AbstractModeType end
+abstract type AbstractSlabPlanetModeSubType1 <: AbstractSlabPlanetModeType end
+struct AMIP_mode <: AbstractModeType end


Julia convention is to use camel case for struct names, so this would be AMIPMode (same for the structs below, e.g. SlabplanetMode

juliasloan25 · 2024-12-16T18:09:23Z

experiments/ClimaEarth/user_io/postprocessing.jl

+        atmos_output_dir,
+        amip_diags_handler,
+        comms_ctx,
+    ) = postprocessing_vars


dir_paths, amip_diags_handler, and comms_ctx are in cs and can be accessed from there in this function

juliasloan25 · 2024-12-16T18:10:10Z

experiments/ClimaEarth/user_io/postprocessing.jl

+        use_coupler_diagnostics,
+        output_default_diagnostics,
+        t_end,
+        conservation_softfail,


Suggested change

conservation_softfail,

We don't need to extract args that won't be used in this function

Same for the slabplanet method - e.g. we don't need the amip_diags_handler for that case

juliasloan25 · 2024-12-16T18:13:27Z

experiments/ClimaEarth/user_io/postprocessing.jl

+    # end #hide
+
+    ## close all AMIP diagnostics file writers
+    !isnothing(amip_diags_handler) && map(diag -> close(diag.output_writer), amip_diags_handler.scheduled_diagnostics)


It would be good for this to be isolated to the AMIP postprocessing method. It needs to be called last (which is maybe why it's here), but we can accomplish both of those things if we call common_postprocessing at the start of each mode-specific postprocess method, and move this to the end of the AMIP method.

juliasloan25 · 2024-12-16T18:15:50Z

experiments/ClimaEarth/user_io/postprocessing.jl

+function postprocess_sim(mode_type::AbstractSlabPlanetModeType, cs, postprocessing_vars)
+    (;
+        dir_paths,
+        plot_diagnostics,


The postprocess_sim methods don't need plot_diagnostics, only common_postprocessing does. It would be good to not retrieve it in methods it isn't needed, and we should check that all the other variables are needed too

juliasloan25 · 2024-12-16T18:40:43Z

experiments/ClimaEarth/user_io/postprocessing.jl

+    # if isinteractive() #hide
+    #     ## clean up for interactive runs, retain all output otherwise #hide
+    #     rm(dir_paths.output; recursive = true, force = true) #hide
+    # end #hide


we can just delete this

juliasloan25

Thanks Teja! Just a note about documentation - we should update the NEWS.md explaining how the postprocessing has changed (e.g. now there are specific methods for each AMIP/slabplanet simulation type), and the new simulation type structs

juliasloan25 · 2024-12-17T20:05:13Z

experiments/ClimaEarth/user_io/postprocessing.jl

+If they exist, perform conservation checks, for any slabplanet simulation, and then call
+`common_postprocessing` to perform common postprocessing tasks that are common to all simulation types.


update docstring since common_postprocessing is called at the beginning of the function now

juliasloan25 · 2024-12-17T20:05:52Z

experiments/ClimaEarth/user_io/postprocessing.jl

+
+    !isnothing(cs.amip_diags_handler) &&


Suggested change

!isnothing(cs.amip_diags_handler) &&

# close all AMIP diagnostics file writers

!isnothing(cs.amip_diags_handler) &&

juliasloan25 · 2024-12-17T20:06:31Z

src/Interfacer.jl

@@ -242,4 +248,24 @@ reinit!(sim::ComponentModelSimulation) = error("undefined reinit! for " * name(s
 # Include file containing the surface stub simulation type.
 include("surface_stub.jl")

+"""
+    AbstractModeType


Suggested change

AbstractModeType

AbstractSimulationMode

juliasloan25 · 2024-12-17T20:07:39Z

src/Interfacer.jl

+struct AMIPMode <: AbstractSimulationMode end
+struct SlabplanetMode <: AbstractSlabplanetSimulationMode end
+struct SlabplanetAquaMode <: AbstractSlabplanetSimulationMode end
+struct SlabplanetTerraMode <: AbstractSlabplanetSimulationMode end
+struct SlabplanetEisenmanMode <: AbstractSlabplanetSimulationMode end


It would be good to include short comments about each of these simulation setups. You can get this information from experiments/ClimaEarth/README.md

juliasloan25 · 2024-12-17T21:11:17Z

experiments/ClimaEarth/run_amip.jl

@@ -327,18 +332,18 @@ if mode_name == "amip"
    CO2_field = Interfacer.update_field!(atmos_sim, Val(:co2), CO2_init)

    mode_specifics = (;
-        name = mode_name,
+        type = sim_mode,


This field isn't used anymore (all uses of cs.mode.name are removed), so we can remove it from mode_specifics

I think the field is used inside of solver_coupler lines 691 and 752
cs.mode.type isa AMIPMode

Oh yeah, good point

juliasloan25

This looks great, thank you!

juliasloan25 · 2024-12-19T01:31:00Z

NEWS.md

+#### Output path updates - PRs [#1058][1], [#1106][2], [#1123][3]
+
+[1]: https://github.com/CliMA/ClimaCoupler.jl/pull/1058
+[2]: https://github.com/CliMA/ClimaCoupler.jl/pull/1106
+[3]: https://github.com/CliMA/ClimaCoupler.jl/pull/1123


It's nice to keep these in the [...] (...) format because it creates a hyperlink (e.g. for PR 1058 here). However as it is in current main, the 2nd and 3rd PRs don't get linked because they're on newlines. Maybe we could do something like this:

Suggested change

#### Output path updates - PRs [#1058][1], [#1106][2], [#1123][3]

[1]: https://github.com/CliMA/ClimaCoupler.jl/pull/1058

[2]: https://github.com/CliMA/ClimaCoupler.jl/pull/1106

[3]: https://github.com/CliMA/ClimaCoupler.jl/pull/1123

#### Output path updates - PRs [#1058](https://github.com/CliMA/ClimaCoupler.jl/pull/1058), [#1106](https://github.com/CliMA/ClimaCoupler.jl/pull/1106), [#1123](https://github.com/CliMA/ClimaCoupler.jl/pull/1123)

The notation I used also creates hyperlinks. See the preview here

I prefer this notation because it avoids making a very long line that wraps around when the source is viewed on most screens. If you prefer the normal hyperlink notation, I'll make the change.

docs/src/interfacer.md

juliasloan25 · 2024-12-19T01:34:14Z

experiments/ClimaEarth/run_amip.jl

@@ -327,18 +332,18 @@ if mode_name == "amip"
    CO2_field = Interfacer.update_field!(atmos_sim, Val(:co2), CO2_init)

    mode_specifics = (;
-        name = mode_name,
+        type = sim_mode,


Oh yeah, good point

src/Interfacer.jl

imreddyTeja · 2024-12-19T17:22:56Z

This looks great, thank you!

Thank you! I wanted to double check that changing the simulation mode types from empty structs to abstract types is ok. I've seen both, so I'm not sure what the convention is. I changed them to abstract types to follow the pattern used by the simulation components

juliasloan25 · 2024-12-19T18:29:55Z

This looks great, thank you!

Thank you! I wanted to double check that changing the simulation mode types from empty structs to abstract types is ok. I've seen both, so I'm not sure what the convention is. I changed them to abstract types to follow the pattern used by the simulation components

I think either is fine in this case! I'm not sure what the convention is either, but from the Julia manual:

An important point to note is that there is no loss in performance if the programmer relies on a function whose arguments are abstract types, because it is recompiled for each tuple of concrete argument types with which it is invoked.

So for our usage of just dispatching off of these types, defining them as abstract types is okay (and empty structs would also be okay)

This commit adds a struct for each different mode_name. A struct is returned by get_cupler_args as mode_type. This is used everywhere mode_name was. The structs do have hierarchy, but the Abstract types should be given better names.

This commit adds a new file, experiments/ClimaEarth/user_io/postprocessing.jl, which contains the `postprocess_sim` function. This function copies the if statements previously at the end of run_amip.jl, but it uses dispatch for the simulation mode.

This is more in line with the ComponentModelSimulation types in Interfacer.jl

imreddyTeja commented Dec 12, 2024

View reviewed changes

imreddyTeja requested a review from juliasloan25 December 16, 2024 16:57

juliasloan25 reviewed Dec 16, 2024

View reviewed changes

imreddyTeja marked this pull request as ready for review December 17, 2024 00:37

imreddyTeja requested a review from juliasloan25 December 17, 2024 17:41

juliasloan25 reviewed Dec 17, 2024

View reviewed changes

imreddyTeja force-pushed the tr/isolate-run_amip-postprocessing branch 2 times, most recently from 07677c1 to b475bad Compare December 18, 2024 17:25

imreddyTeja requested a review from juliasloan25 December 18, 2024 23:47

juliasloan25 approved these changes Dec 19, 2024

View reviewed changes

imreddyTeja added 8 commits December 19, 2024 12:22

Replace mode_name with mode_type in run_amip

37922f7

This commit adds a struct for each different mode_name. A struct is returned by get_cupler_args as mode_type. This is used everywhere mode_name was. The structs do have hierarchy, but the Abstract types should be given better names.

Isolate run_amp.jl postprocessing

52d1149

This commit adds a new file, experiments/ClimaEarth/user_io/postprocessing.jl, which contains the `postprocess_sim` function. This function copies the if statements previously at the end of run_amip.jl, but it uses dispatch for the simulation mode.

Improve comments and docs in postprocessing.jl

a530c6d

Add docstrings for simulation mode types

540256f

Fix NEWS.md formatting

f61b775

Add News.md section for simulation mode structs and postprocessing

e095aa2

Change simulation mode structs to abstract types

3bf2682

This is more in line with the ComponentModelSimulation types in Interfacer.jl

Fix documentation errors

26c8fb8

imreddyTeja force-pushed the tr/isolate-run_amip-postprocessing branch from a04cab0 to 26c8fb8 Compare December 19, 2024 20:24

imreddyTeja merged commit 958f042 into main Dec 20, 2024
11 checks passed

imreddyTeja deleted the tr/isolate-run_amip-postprocessing branch December 20, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tr/isolate run amip postprocessing #1120

Tr/isolate run amip postprocessing #1120

imreddyTeja commented Dec 12, 2024 •

edited

Loading

imreddyTeja Dec 12, 2024

imreddyTeja Dec 12, 2024

juliasloan25 Dec 16, 2024 •

edited

Loading

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 Dec 16, 2024

juliasloan25 left a comment

juliasloan25 Dec 17, 2024

juliasloan25 Dec 17, 2024

juliasloan25 Dec 17, 2024

juliasloan25 Dec 17, 2024

juliasloan25 Dec 17, 2024

imreddyTeja Dec 17, 2024

juliasloan25 Dec 19, 2024

juliasloan25 left a comment

juliasloan25 Dec 19, 2024

imreddyTeja Dec 19, 2024

juliasloan25 Dec 19, 2024

imreddyTeja commented Dec 19, 2024

juliasloan25 commented Dec 19, 2024

		@@ -1,5 +1,21 @@
		import YAML

		abstract type AbstractModeType end

	Perfrom postprocessing common to all simulation types.
	Perform postprocessing common to all simulation types.

		If they exist, perform conservation checks, for any slabplanet simulation, and then call
		`common_postprocessing` to perform common postprocessing tasks that are common to all simulation types.

Tr/isolate run amip postprocessing #1120

Tr/isolate run amip postprocessing #1120

Conversation

imreddyTeja commented Dec 12, 2024 • edited Loading

Purpose

To-do

Content

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliasloan25 Dec 16, 2024 • 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

juliasloan25 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

juliasloan25 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

imreddyTeja commented Dec 19, 2024

juliasloan25 commented Dec 19, 2024

imreddyTeja commented Dec 12, 2024 •

edited

Loading

juliasloan25 Dec 16, 2024 •

edited

Loading