EAMxx: Give option for for tracers to be advected only by dynamics #6789

tcclevenger · 2024-12-02T16:23:21Z

Processes can choose for a tracer to be advected only by dynamics (not by SHOC) by passing turbulence_advected=false to add_tracer(). Guards set up to ensure all tracers requests are consistent among processes.

Moved over from E3SM-Project/scream#3085. Currently working on issue of tracer ordering between PG2 and GLL grids.

[non-BFB for MAMxx]

bartgol · 2024-12-02T22:10:03Z

@tcclevenger Just to clarify: you said you're still working on ordering of tracers for PG2 and DYN grids, so is this PR not yet ready?

tcclevenger · 2024-12-02T22:18:27Z

@tcclevenger Just to clarify: you said you're still working on ordering of tracers for PG2 and DYN grids, so is this PR not yet ready?

Yes. Do we have a WIP equiv over here so I don't let the AT run while I work on getting a fix in?

bartgol · 2024-12-02T22:23:16Z

@tcclevenger Just to clarify: you said you're still working on ordering of tracers for PG2 and DYN grids, so is this PR not yet ready?

Yes. Do we have a WIP equiv over here so I don't let the AT run while I work on getting a fix in?

No we don't. But as soon as you push commits, you can click on the checks tab, and cancel all the jobs (one workflow at a time, unfortunately), if you know you're not done anyways...

Edit: But we could modify our workflows jobs so that, for pull_request events, they only run if the PR is not in draft mode...

rljacob · 2024-12-02T22:26:17Z

What if you change it to draft PR ?

bartgol · 2024-12-02T22:28:04Z

What if you change it to draft PR ?

I saw your reply just as I was editing my answer... Unfortunately, gh actions pull_request event still triggers, even if the PR is in draft mode (a debatable design choice, imho). The only way to skip them is to do something like

jobs:
  my-job:
    if: ${{ github.event_name != "pull_request" || github.event.pull_request.draft == "false" }}
    ...

bartgol · 2024-12-02T22:34:01Z

@tcclevenger i removed the reviewer request. When you're ready, add them back, so gh pings me. If you'd rather us review right now, even before you fix the PG2-DYN issue, feel free to re-add me/us (I know balwinder is out for a month though).

rljacob · 2024-12-19T18:19:00Z

@tcclevenger what is the status?

tcclevenger · 2024-12-19T18:20:32Z

Still working on a fix for mismatch tracer indices between PG2 and GLL grid

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

rljacob · 2025-01-09T18:27:34Z

@tcclevenger any progress on fix?

tcclevenger · 2025-01-09T18:38:42Z

@tcclevenger any progress on fix?

Yes, this is what I'm currently working on. Will update as I go.

rljacob · 2025-01-28T19:08:28Z

@tcclevenger please try to make this old PR a priority.

ambrad · 2025-01-28T19:22:38Z

@rljacob is it OK if we switch this to draft?

rljacob · 2025-01-28T19:26:22Z

Sure thats fine.

ambrad · 2025-01-28T19:27:51Z

@tcclevenger would you switch this to Draft if it's not ready? We'll try out that workflow.

tcclevenger · 2025-01-28T19:37:41Z

@tcclevenger please try to make this old PR a priority.

This is what I am currently working on, but the work is in a different branch (it contains a fundamental change to allocating groups of fields that doesn't make sense to throw onto this branch). Yes, I agree draft makes more sense for this PR.

bartgol · 2025-03-10T20:42:38Z

Seems that some of the other CIME cases are erroring out trying to start the container? Have we seen that elsewhere?

I'm guessing the error is this one?

  toomanyrequests: {"errors":[{"code":"TOOMANYREQUESTS","message":"retry-after: 51.451683ms"}]}

I have never seen it. I suppose we had too many concurrent PRs that needed to be tested, and all the gh-hosted runners tried to access the container registry at the same time? Should be safe to re-run the failed tests manually.

singhbalwinder

Looks great from the MAM4xx side! Thanks, Conrad for adding this very important feature!

ambrad

I approve as long as the "[non-BFB for MAMxx]" status remains true (I think it is, but just want to be sure).

bartgol

So there wasn't much to do in FieldManager? That's nice. With this branch, did you try to print the tracer group content, and verify that the non-turb-advected tracers are all at the end?

I have some thoughts on the design. Not really requests, just discussions sparks.

bartgol · 2025-03-12T20:05:47Z

components/eamxx/src/control/atmosphere_driver.cpp

+      if (not ekat::contains(req.groups, "tracers")) return;
+
+      std::string fname = req.fid.name();
+      if (tracer_requests.find(fname) == tracer_requests.end()) {


I think this check is pointless. The else branch should work regardless, no?

bartgol · 2025-03-12T20:13:22Z

components/eamxx/src/control/atmosphere_driver.cpp

+
+    // Go through the map entry for each tracer and check that every one
+    // has the same request for turbulence advection.
+    for (auto fr : tracer_requests) {


Say MAM requests field A, specifying that it should not be turb advected. Another process uses field A, so it requests it, but it has no knowledge of what turb advection is, or at least it doesn't care (the choice may be relevant for MAM, but the rest of the atm doesn't care). In this case, wouldn't the two request conflict?

Maybe we should turn this into an enum, like

enum TracerAdvection { DynOnly, DynAndTurb, Default, };

(the names are up for debate, ofc). In this case, the default would be Default. If all procs use "default", we do both dyn and turb advection, as before. If all procs use "default", but some ask for "DynOnly", we do dyn only. The error only arises if some procs expicitly ask for "DynOnly" and others explicitly ask for "DynAndTurb".

Maybe I'm over thinking it, but there is no way, with a bool, to check if some values for turb_advected_tracers are true b/c the customer asked for true, or simply b/c they didn't care, and relied on the default.

The enum may make things complicated, in which case we may also consider whether this check is needed. That is, as long as there is ONE request that says "don't do turb advection", then we don't add it to the turb advected tracers.

Maybe I need to better understand the case we are trying to guard ourselves from...

OTOH, using enums in the add_tracer calls make the code more self-explanatory...so maybe switching to an enum is a bit clearer?

add_tracer<Required>("blah",units, grid,1,false);

vs

add_tracer<Required>("blah",units, grid,1,NoTurbAdvection);

I may be leaning towards getting rid of this check. I think (but will verify) that the changes that evolved in the FM since I wrote this check actually make it a bit redundant? It should error out with inconsistent requests, we just may not get the output about which processes conflict.

With regards to the enums, I may be having a change of heart. I've always avoided in the case where a bool was all that's needed, but these signatures definitely are less clear and add_tracer and add_group are certainly part of the code that users will interface with if they write a process. Will change them back.

Same here: I often though "why adding a new type when a bool flag does it?". But code clarity with enums is on a whole other level.

Follow up: We need this check given how group requests do group logic, namely, if a single processes says "I want A in group G" then A gets added to G, even if some processes intentionally did not add A to G.

I think this option is more of "I want to explicitly state that A should not be in group G" and then these checks ensure no one added it.

components/eamxx/src/physics/shoc/eamxx_shoc_process_interface.cpp

- AtmosphereProcess::add_tracer() now takes in bool turbulence_advected which dictates if shoc advects tracer or ignores - SHOC requests group "turbulence_advected_tracers", not full "tracers" array - FieldRequest includes a "calling_process" var for error output

…lt value (false)

This reverts commit 6237c87. Adds back the enum for bundling

tcclevenger · 2025-03-13T15:37:42Z

With this branch, did you try to print the tracer group content, and verify that the non-turb-advected tracers are all at the end?

Yes, for PG2, I verified GLL<->PG2 indices matched, and that non-turb-advected tracers came last.

bartgol · 2025-03-13T15:38:29Z

components/eamxx/src/dynamics/homme/eamxx_homme_process_interface.cpp


  add_tracer<Updated >("qv", m_phys_grid, kg/kg, N);
-  add_group<Updated>("tracers",pgn,N, true);
+  add_group<Updated>("tracers",pgn,N, Bundling::Required);


Since we're bringing the enum back, I may as well ask: do you think the name Bundling is clear? Maybe we could change it into something else. Some ideas:

enum class GroupAllocation { Contiguous, NotContiguous }; enum class MonolithicGroup { Required, NotRequired }; enum class ContiguousGroup { Requred, NotRequired };

I don't know. I wonder if the word "bundle" is a bit obscure, and we can find something more self-explanatory? If you have other name ideas, feel free to bounce them off us. This is the right time to discuss this, before we roll it out again.

The user potentially needs to know what group.m_bundle is, maybe we would need to think about changing that name too?

I like Monolithic the best. To me non-contiguous implies that "I can access the entire group, it just won't be contiguous" where the only way to access a view of the entire group is bundled case.

Yes, if we decide on a name change, we should change it everywhere...

What if we used

enum class MonolithicAlloc { Required, NotRequired };

? I agree that the word "monolithic" carries a clear meaning. But I also like to have the word "allocation" (or something reminescent of it) somwhere.

What would you then call the bundle? group.m_monolithic? group.m_monolithic_field? group.m_whole? Something else? I like m_monolithic_field, and maybe rename m_fields into m_individual_fields?

Feel free to propose others...

We can also merge as is, and follow up with a better name convention right after...

I like m_monolithic_field and m_individual_fields. I think this is much clearer than "bundling". I've updated all the comments that refer to bundling as well.

tcclevenger · 2025-03-13T19:58:21Z

Confirmed #6981 did not introduce non-BFB fails on PM-GPU, so we can merge this once we settle on a "bundling" name

- Bundling enum -> MonolithicAlloc - m_bundle field -> m_monolithic_field - m_fields (in field group) -> m_individual_fields - All comments updated

tcclevenger · 2025-03-14T01:05:28Z

I decided to update bundling name instead of merging since I'm off work tomorrow and so don't want to merge tonight. Assuming there are no fails/change requests with the naming, I'll merge first thing next week (Sunday night, even, if I remember...).

@singhbalwinder Thanks for your patience with this PR! Ended up taking much more time than anticipated..

singhbalwinder · 2025-03-14T01:23:34Z

No worries at all—I completely understand that these things can take much longer than expected. I appreciate the time and effort you’ve put into this. Merging next week is fine.

rljacob · 2025-03-17T14:50:04Z

Is this ready?

tcclevenger · 2025-03-17T14:51:58Z

Is this ready?

Yes, going to merge today.

rljacob · 2025-03-17T15:22:38Z

Hang on. I think we want to do the Kokkos branch today.

rljacob · 2025-03-17T16:56:27Z

Kokkos branch delayed so go ahead.

… into next (PR #6789) Processes can choose for a tracer to be advected only by dynamics (not by SHOC) by passing turbulence_advected=false to add_tracer(). Guards set up to ensure all tracers requests are consistent among processes. [non-BFB for MAMxx]

tcclevenger added non-BFB PR makes roundoff changes to answers. EAMxx Issues related to EAMxx labels Dec 2, 2024

tcclevenger requested review from bartgol and singhbalwinder December 2, 2024 16:23

tcclevenger self-assigned this Dec 2, 2024

bartgol removed request for bartgol and singhbalwinder December 2, 2024 22:33

rljacob mentioned this pull request Dec 12, 2024

Allow for tracer to not be advected by SHOC E3SM-Project/scream#3085

Closed

tcclevenger commented Jan 8, 2025

View reviewed changes

components/eamxx/src/physics/mam/eamxx_mam_aci_process_interface.hpp Outdated Show resolved Hide resolved

rljacob requested a review from ambrad January 28, 2025 19:08

rljacob marked this pull request as draft January 28, 2025 19:26

tcclevenger mentioned this pull request Feb 7, 2025

Unify FM over all grids #6981

Merged

tcclevenger force-pushed the tcclevenger/eamxx/add_tracer_with_turbulence_advection branch 3 times, most recently from 785340d to 8d5f18b Compare March 6, 2025 17:50

singhbalwinder approved these changes Mar 12, 2025

View reviewed changes

tcclevenger force-pushed the tcclevenger/eamxx/add_tracer_with_turbulence_advection branch from cb70a4c to 4269e18 Compare March 12, 2025 19:20

tcclevenger requested a review from singhbalwinder March 12, 2025 19:21

ambrad approved these changes Mar 12, 2025

View reviewed changes

singhbalwinder approved these changes Mar 12, 2025

View reviewed changes

bartgol reviewed Mar 12, 2025

View reviewed changes

tcclevenger force-pushed the tcclevenger/eamxx/add_tracer_with_turbulence_advection branch from b690428 to 5a4eb82 Compare March 13, 2025 14:26

tcclevenger added 5 commits March 13, 2025 10:28

Use get_strided_layout for shoc tracers

2df42fe

Store FieldGroups to avoid recomputing

b1acce2

Change turbulence_advected->no_turbulence_advection, and give a defau…

6bd2c91

…lt value (false)

Revert "Convert bundling to bool"

f957603

This reverts commit 6237c87. Adds back the enum for bundling

tcclevenger force-pushed the tcclevenger/eamxx/add_tracer_with_turbulence_advection branch from 5a4eb82 to f957603 Compare March 13, 2025 14:32

Use enum for advecting decidion

94b7259

tcclevenger requested a review from bartgol March 13, 2025 15:37

bartgol reviewed Mar 13, 2025

View reviewed changes

bartgol approved these changes Mar 13, 2025

View reviewed changes

Several renames wrt bundling

c3fb462

- Bundling enum -> MonolithicAlloc - m_bundle field -> m_monolithic_field - m_fields (in field group) -> m_individual_fields - All comments updated

tcclevenger merged commit 1e40ce2 into E3SM-Project:master Mar 17, 2025
13 of 29 checks passed

tcclevenger deleted the tcclevenger/eamxx/add_tracer_with_turbulence_advection branch March 17, 2025 19:26

EAMxx: Give option for for tracers to be advected only by dynamics #6789

EAMxx: Give option for for tracers to be advected only by dynamics #6789

Uh oh!

Conversation

tcclevenger commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartgol commented Dec 2, 2024

Uh oh!

tcclevenger commented Dec 2, 2024

Uh oh!

bartgol commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rljacob commented Dec 2, 2024

Uh oh!

bartgol commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartgol commented Dec 2, 2024

Uh oh!

rljacob commented Dec 19, 2024

Uh oh!

tcclevenger commented Dec 19, 2024

Uh oh!

Uh oh!

rljacob commented Jan 9, 2025

Uh oh!

tcclevenger commented Jan 9, 2025

Uh oh!

rljacob commented Jan 28, 2025

Uh oh!

ambrad commented Jan 28, 2025

Uh oh!

rljacob commented Jan 28, 2025

Uh oh!

ambrad commented Jan 28, 2025

Uh oh!

tcclevenger commented Jan 28, 2025

Uh oh!

bartgol commented Mar 10, 2025

Uh oh!

singhbalwinder left a comment

Choose a reason for hiding this comment

Uh oh!

ambrad left a comment

Choose a reason for hiding this comment

Uh oh!

bartgol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcclevenger commented Mar 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcclevenger commented Dec 2, 2024 •

edited

Loading

bartgol commented Dec 2, 2024 •

edited

Loading

bartgol commented Dec 2, 2024 •

edited

Loading