EAMxx: abstract MAMGenericInterface and check intervals in MAM4xx fields. #7054

odiazib · 2025-02-22T00:12:32Z

We implemented the MAMGenericInterface class, derived from AtmosphereProcess, to eliminate duplicate code in the MAM4xx processes. Blocks of code that add tracers to populate the data structures for wet and dry aerosols and atmospheric data, and populate these common structures in the MAM4xx processes, were refactored into methods within MAMGenericInterface. Additionally, we moved the pre/postprocess functionality to this new class where possible.

Furthermore, we added a method in MAMGenericInterface to check interval limits for all fields computed in MAM4xx. The physical limits for the MAM4xx fields are defined in physical_limits.hpp. I have added FIXMEs to values for which I do not have accurate estimates.

eamxx-v1 / cpu-gcc / SMS_Ln3.ne4pg2_oQU480.F2010-MMF2 is failing in master branch.

[BFB]

…e base class of MAM (MAMGenericInterface).

…les that are used in wet and dry struct data.

Use MAMGenericInterface::add_aerosol_tracers to remove duplicate code from all MAM4xx interfaces.

I also renamed m_grid to grid_ in the wetscav interface.

Implement MAMGenericInterface::populate_wet_and_dry_atm and remove duplicate code from all MAM4xx processes except optics, where we have a discrepancy with pseudo_density and pseudo_density_dry.

…n possible."

…ods in MAMGenericInterface.

bartgol

Nice PR. I Love when the deleted lines greatly outnumber the added ones!

I have a few comments/suggestions.

components/eamxx/src/physics/mam/eamxx_mam_generic_process_interface.hpp

bartgol · 2025-02-28T16:35:32Z

components/eamxx/src/physics/mam/eamxx_mam_generic_process_interface.hpp

+
+namespace scream {
+class MAMGenericInterface : public scream::AtmosphereProcess {
+ public:


I think you have mixed up a bit what should be public and what should be private/protected. The only public things should be the constructor and the type() method. Everything else should be protected (not private).

I added all methods from the mam4xx base class under protected; otherwise, I will get a compilation error because the methods are private.

components/eamxx/src/physics/mam/eamxx_mam_generic_process_interface.cpp

bartgol · 2025-02-28T16:45:58Z

components/eamxx/src/physics/mam/eamxx_mam_generic_process_interface.cpp

+    // NOT advected
+    const std::string cld_nmr_field_name =
+        mam_coupling::cld_aero_nmr_field_name(mode);
+    add_field<Updated>(cld_nmr_field_name, scalar3d_mid, n_unit, grid_name);


Are these not tracers that dynamic should advect? They are not added via add_tracer, and no "tracer" string is used to tell the field mgr to add them to the tracer group. Hence, dyn will not advect them. Is this desired?

bartgol · 2025-02-28T16:47:02Z

components/eamxx/src/physics/mam/eamxx_mam_generic_process_interface.cpp

+  using namespace ekat::units;
+  auto q_unit           = kg / kg;  // units of mass mixing ratios of tracers
+  auto n_unit           = 1 / kg;   // units of number mixing ratios of tracers
+  const auto &grid_name = grid_->name();


You can rm grid_name and scalar3d_mid, as they are unused (since add_tracer hard-codes layout/grid_name from the input grid obj).

In this block of code, we are using add_field, which utilizes grid_name and scalar3d_mid. @singhbalwinder, should we use add_tracer instead of add_field?

components/eamxx/src/physics/mam/eamxx_mam_generic_process_interface.cpp

bartgol · 2025-02-28T17:18:36Z

components/eamxx/src/physics/mam/physical_limits.hpp

+      {"ni", {0, 0.1e10}},
+      {"nmr", {0, 1e13}},
+      {"mmr", {-1e-10, 1e-2}},
+      {"omega", {-1e10, 1e10}},       // FIXME


I think you just put [-1e10,1e10] as a placeholder as you were coding, but I would encourage you to fix as many of these ranges as possible. Some allow for obviously wrong values (e.g., landfrac or concentrations that are negative or >1), while others are way too large.

As an alternative: you could add checks for the quantities that MAM is in charge of. That is, no need to provide a range for state var, or other variables that omega only uses as inputs. The assumption being that whoever computes those vars may have logic in place for postcondition checks.

If you go this route, you will have to modify MAMGenericInterface::add_interval_check, so that it adds a check only for the vars for which MAM has a range.

Thinking about it, I think the alternative idea may be best. It will allow you to add checks one by one, if you think they are important. So, to recap:

only provide ranges for the output vars, and only for those that you think it's important to check

in add_interval_checks, only add checks if MAM is storing a valid range

I think you just put [-1e10,1e10] as a placeholder as you were coding, but I would encourage you to fix as many of these ranges as possible. Some allow for obviously wrong values (e.g., landfrac or concentrations that are negative or >1), while others are way too large.

Yes, I added several FIXME comments so that @singhbalwinder and others can help identify a reasonable range for these variables. For some variables, I increased the value to a very large number to ensure the tests pass. If any tests generate unphysical values, we can address those in a follow-up PR.

As an alternative: you could add checks for the quantities that MAM is in charge of. That is, no need to provide a range for state var, or other variables that omega only uses as inputs. The assumption being that whoever computes those vars may have logic in place for postcondition checks.

If you go this route, you will have to modify MAMGenericInterface::add_interval_check, so that it adds a check only for the vars for which MAM has a range.

We added these interval checks to make it easier to detect failures in the cime case. For this purpose, it would be beneficial to have a way to control these checks on a per-process basis. For example, we could run a cime case and only check interval values for ACI. However, I would like to invite @singhbalwinder to comment on this

components/eamxx/tests/single-process/mam/aci/input.yaml

mahf708

Not sure what you're looking for from reviewers. Do you want general feedback on the design approach or are you set on this design and you just want feedback on just integrating it?

For the latter (just integrating it), generally fine. Some minor comments are: move the limits from cpp to a yaml/csv file and just read them (and/or add them somewhere under constants/share type of setup while at it). It would also be good to cross-check that the limits you're applying are consistent with other processes (rrtmgp and other process do apply some limits in debug mode and at runtime).

The rest of the comment is about the former (total redesign). I think this may warrant a bigger conversation as some of these things you're adding here may belong elsewhere. it's also not readily clear to me how you envision MAM4xx will be run in the future. For example, while I think it is a good idea to isolate subprocess components (say optics vs wet scavenging), it is a bit misleading to have like 7+ MAM4xx processes when MAM4xx is just one actual physics process/model. I am not a fan of the ordering and how stuff was woven into EAM in terms of injecting parts of MAM in certain places and injecting others elsewhere, etc., and I'd like to encourage a better strategy.

So if you're interested in comments about the overall redesign, I think key issues to consider are what in here is really specific to mam4xx and what isn't --- what can we abstract out all the up to AtmosphereProcess and what we need to keep in this MAMGenericInterface process. Having several layers of abstraction will likely come back to haunt us, even if it is convenient now.

Additionally, if we want to retain the "isolated" process setup, I don't see how the generic methods without arguments are consistent with the design of isolated process. I haven't looked carefully, but you're likely introducing more things than needed by any individual process (e.g., do all processes actually need what's under MAMGenericInterface::add_tracers_wet_and_dry_atm? I doubt it).

I could go on, but maybe you're not interested in a design discussion and you just want this integrated asap 😉 let us know either way what you want us to give you feedback on, because there are two distinct aspects of this PR and you will get (at least from me) totally two different responses depending on what you prefer :)

1. Remove AtmosphereProcessType from derived classes. 2. Move the header to the *.cpp file and add 'protected' to the methods from the base class. 3. Rename the method get_aerosol_gas_map and delete any unused methods. 4. We do not need to invoke get_field_in. 5. Rename mam4_check_fields_intervals to create_fields_interval_checks. 6. Add create_fields_interval_checks to namelist_default_eamxx.

odiazib force-pushed the pre_post_conditionals branch 3 times, most recently from 4b825d3 to ea43ba8 Compare February 22, 2025 21:07

odiazib added 11 commits February 24, 2025 10:10

Base class and physical limists for constituent_fluxes_interface.

e91f954

EAMxx: Move the block of code with add_tracer calls to a method in th…

c6ccca3

…e base class of MAM (MAMGenericInterface).

Creating a method in MAMGenericInterface to check intervals in variab…

cac3d6b

…les that are used in wet and dry struct data.

EAMxx: Checking intervals for all fields in MAM4xx.

39629da

EAMxx: Remove duplicate code.

3485ef8

Use MAMGenericInterface::add_aerosol_tracers to remove duplicate code from all MAM4xx interfaces.

EAMxx: Remove duplicate code.

98488ba

I also renamed m_grid to grid_ in the wetscav interface.

EAMxx: Implement MAMGenericInterface::populate_wet_and_dry_atm.

35cf256

Implement MAMGenericInterface::populate_wet_and_dry_atm and remove duplicate code from all MAM4xx processes except optics, where we have a discrepancy with pseudo_density and pseudo_density_dry.

EAMxx: Remove duplicate code in mam4xx optics.

3466b20

Move Preprocess and Postprocess structures to MAMGenericInterface whe…

39cd204

…n possible."

EAMxx: Using std::string instead of char*. Removing ATM process header.

913344d

EAMxx:Renaming function names, adding comments, and clang-format.

5450200

odiazib force-pushed the pre_post_conditionals branch from 92df9c3 to 5450200 Compare February 24, 2025 18:20

odiazib added BFB PR leaves answers BFB EAMxx PRs focused on capabilities for EAMxx MAM4xx MAM4xx related changes labels Feb 24, 2025

odiazib requested a review from singhbalwinder February 24, 2025 18:34

EAMxx: Converting pre and post processes from private structs to meth…

037b06b

…ods in MAMGenericInterface.

odiazib force-pushed the pre_post_conditionals branch from c02b639 to 92bbc32 Compare February 26, 2025 04:20

odiazib added 2 commits February 26, 2025 08:41

EAMxx: Changing the lower limit of mmr to make the SP test pass.

a971404

EAMxx: Fixing runtime error in CUDA tests.

cb0172d

odiazib force-pushed the pre_post_conditionals branch from 92bbc32 to cb0172d Compare February 26, 2025 15:41

odiazib marked this pull request as ready for review February 27, 2025 17:43

odiazib requested review from bartgol and mjschmidt271 February 27, 2025 17:43

bartgol reviewed Feb 28, 2025

View reviewed changes

rljacob assigned singhbalwinder Mar 3, 2025

rljacob removed the request for review from singhbalwinder March 3, 2025 04:50

odiazib requested a review from mahf708 March 3, 2025 17:36

mahf708 reviewed Mar 3, 2025

View reviewed changes

mahf708 changed the title ~~EAMxx: Check intervals in MAM4xx fields.~~ EAMxx: abstract MAMGenericInterface and check intervals in MAM4xx fields. Mar 3, 2025

mahf708 added code usability code cleanup labels Mar 3, 2025

odiazib force-pushed the pre_post_conditionals branch from f726c00 to 082f79d Compare March 3, 2025 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EAMxx: abstract MAMGenericInterface and check intervals in MAM4xx fields. #7054

EAMxx: abstract MAMGenericInterface and check intervals in MAM4xx fields. #7054

odiazib commented Feb 22, 2025 •

edited

Loading

bartgol left a comment

bartgol Feb 28, 2025

odiazib Mar 3, 2025

bartgol Feb 28, 2025

bartgol Feb 28, 2025

odiazib Mar 3, 2025 •

edited

Loading

bartgol Feb 28, 2025

bartgol Feb 28, 2025

bartgol Feb 28, 2025

odiazib Mar 3, 2025

odiazib Mar 3, 2025

mahf708 left a comment

EAMxx: abstract MAMGenericInterface and check intervals in MAM4xx fields. #7054

Are you sure you want to change the base?

EAMxx: abstract MAMGenericInterface and check intervals in MAM4xx fields. #7054

Conversation

odiazib commented Feb 22, 2025 • edited Loading

bartgol 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

odiazib Mar 3, 2025 • 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

mahf708 left a comment

Choose a reason for hiding this comment

odiazib commented Feb 22, 2025 •

edited

Loading

odiazib Mar 3, 2025 •

edited

Loading