EAMxx: add p3 process rates as outputs #6938

mahf708 · 2025-01-24T15:32:24Z

adds p3 process rates as outputs controlled by a runtime flag.

this impl is kind of hacky because we don't yet have the facility to request output fields adhoc and these fields are buried deep in the p3 impl; in the future, this could be edited to be done more smartly, but for now, this may be just enough...

github-actions · 2025-01-24T15:34:33Z

PR Preview Action v1.6.0
🚀 View preview at https://E3SM-Project.github.io/E3SM/pr-preview/pr-6938/
Built to branch `gh-pages` at 2025-01-24 15:34 UTC. Preview will be ready when the GitHub Pages deployment is complete.

mahf708 · 2025-01-24T15:40:27Z

@hassanbeydoun + @brhillman, after we get a review from Luca and/or Jim (or other reviewers), you can test this with dpxx or short runs. In the meanwhile, let me know if I missed something that we might as well add while at it?

I expect Luca and Jim to request some edits, because this was a rushed impl. I don't have much time to make this super fancy or complicated, because the p3 source code is pretty convoluted. Actually, dealing with the hardcoded unit tests was probably the most challenging and wasted most of my time. I anticipate we will have to rewrite much of p3 and shoc in the future, because I don't think they're optimal (I'd argue both performance and code readability are much below what I'd think we should have)

I know Cornard is working on a way to make this type of work less painful (i.e., by making adhoc diagnostic requests through the field manager). Once that's ready, we can rewrite this impl. But I wanted to get this out because I know people are interested in scientific analysis/validation around these rates asap.

mahf708 · 2025-01-24T15:41:10Z

components/eamxx/src/physics/p3/eamxx_p3_process_interface.cpp

+    // if not, let's just use the buffer for the unused? field
+    // TODO: check if this is actually okay and doesn't have uintended consequences
+    // if we are not outputing these fields, we really don't care about their values
+    // but would this have side effects or memory issues? idk
+    // TODO: maybe just use more buffer and assign stuff?
+    history_only.P3_qr2qv_evap   = m_buffer.unused;
+    history_only.P3_qi2qv_sublim = m_buffer.unused;
+    history_only.P3_qc2qr_accret = m_buffer.unused;


this may be stupid as hell, so we might need to add more to the buffer or figure out an alternative

jgfouca

Looks fine. If p3_extra_diags is off, we could just leave the views uninitialized. Then, in p3_main_part2, you could check if P3_qr2qv_evap is a "real" view (initialized and allocated) before doing the sets.

bartgol

Looks good. Would you mind opening an issue regarding p3_extra_diags, saying what should be changed (e.g., avoid calc if diags are not requested)? It will likely fall into oblivion, but at least we'd have tried :)

mahf708 · 2025-01-27T17:01:18Z

components/eamxx/cime_config/namelist_defaults_scream.xml

@@ -227,6 +227,7 @@ be lost if SCREAM_HACK_XML is not enabled.
      <set_cld_frac_l_to_one type="logical" doc="set P3 input liquid cloud fraction to 1 everywhere">false</set_cld_frac_l_to_one>
      <set_cld_frac_r_to_one type="logical" doc="set P3 input rain cloud fraction to 1 everywhere"  >false</set_cld_frac_r_to_one>
      <set_cld_frac_i_to_one type="logical" doc="set P3 input ice cloud fraction to 1 everywhere"   >false</set_cld_frac_i_to_one>
+      <p3_extra_diags type="logical" doc="Extra P3 diagnostics">false</p3_extra_diags>


@bartgol is this enough for documentation or would you like something else addressed?

Since you are offering... I suppose you could be more verbose. Here, the description seems just a spell out of the short code name. Maybe something like "Enable calculation of additional diagnostic output from P3"?

Looks good. Would you mind opening an issue regarding p3_extra_diags, saying what should be changed (e.g., avoid calc if diags are not requested)? It will likely fall into oblivion, but at least we'd have tried :)

Ah, sorry, I misunderstood your comment. Well, the larger issue is the convoluted code, but if you and Jim are happy with how I did it, it is time to merge it and let the p3 warriors (not me) do their testing

Let's just merge then. I am marking this as resolved. I think the "P3 diagnostics" will be something only select few will be using.

Well, we have to fix a few fails first.

REQUIRE( isds_baseline[i].T_atm[k] == isds_cxx[i].T_atm[k] ) with expansion: 267.12479f == 265.32697f

☠️

When I ran the basic p3_tests and p3_sk_tests, they passed fine on pm-gpu (sp and dbg). I obviously didn't try the p3_tests_omp1 etc.

mahf708 · 2025-01-28T02:57:31Z

components/eamxx/src/physics/p3/tests/p3_main_unit_tests.cpp

-      d.nevapr, d.qr_evap_tend, d.vap_liq_exchange, d.vap_ice_exchange, d.liq_ice_exchange, d.pratot,
+      d.nevapr, d.qr_evap_tend, d.vap_liq_exchange, d.vap_ice_exchange, d.liq_ice_exchange, 
+      d.P3_qr2qv_evap, d.P3_qi2qv_sublim, d.P3_qc2qr_accret, d.P3_qc2qr_autoconv, d.P3_qv2qi_vapdep, d.P3_qc2qi_berg, d.P3_qc2qr_ice_shed, d.P3_qc2qi_collect, d.P3_qr2qi_collect, d.P3_qc2qi_hetero_freeze, d.P3_qr2qi_immers_freeze, d.P3_qi2qr_melt,
+      d.pratot,


The error(s) happen(s) in the comparison a few lines below...

/home/runner/_work/E3SM/E3SM/components/eamxx/src/physics/p3/tests/p3_main_unit_tests.cpp:233: FAILED: REQUIRE( isds_baseline[i].T_atm[k] == isds_cxx[i].T_atm[k] ) with expansion: 279.9509309491 == 283.0331820996

I am at a loss what might be causing this...

mahf708 · 2025-01-28T03:03:41Z

components/eamxx/src/physics/p3/tests/infra/p3_test_data.hpp

@@ -708,7 +710,7 @@ struct P3MainPart2Data : public PhysicsTestData

 struct P3MainPart3Data : public PhysicsTestData
 {
-  static constexpr size_t NUM_ARRAYS = 34;
+  static constexpr size_t NUM_ARRAYS = 47;


Suggested change

static constexpr size_t NUM_ARRAYS = 47;

static constexpr size_t NUM_ARRAYS = 34;

mahf708 · 2025-01-28T03:09:04Z

@bartgol @jgfouca, I am not sure what might be causing the baseline DIFFs here. (I confirmed; all the DIFFs are baseline diffs; the MMF2 test is a different cookie, so forget it for now.)

Do you spot anything that can change answers somewhere? I am not sure. Could it be an indexing issue in the tests?

Note: the issue seems confined to the unit tests; i.e., the actual model runtime is unaffected. I will probably be inclined to re-bless/re-generate these unit tests instead of dwelling too much on it. I will bring it up in the dev call on tues

mahf708 · 2025-01-28T15:56:08Z

After looking a little more at the problems this morning, I concluded the following:

the fixes will likely take quite a bit of my time
the fixes are outside the main eamxx runtime (in the unit tests and in the pam driver; both of which are outside my interest and scope of willingness to spend much time chasing issues)

I will therefore close this PR. People interested in these tendencies/diagnostics can simply run with this PR as a patch and the results should be trustworthy. Integrating this into the mainline code is a bit of a hassle ...

whannah1 · 2025-01-28T16:00:24Z

the fixes are outside the main eamxx runtime (in the unit tests and in the pam driver; both of which are outside my interest and scope of willingness to spend much time chasing issues)

If it helps I can take ownership of the changes in PAM so you wouldn't need to worry about those tests.

mahf708 · 2025-01-28T16:06:42Z

the fixes are outside the main eamxx runtime (in the unit tests and in the pam driver; both of which are outside my interest and scope of willingness to spend much time chasing issues)

If it helps I can take ownership of the changes in PAM so you wouldn't need to worry about those tests.

Too late; the MMF2 test is now part of eamxx-v1 testing (https://github.com/E3SM-Project/E3SM/actions/runs/12952656866).

In general, I am not sure if EAMxx developers should deal with the burden of maintaining the fragile PAM/MMF2 setup, but that's above my pay grade (if I can deal with it quickly, I will; if not, I call it quits). I just don't really have much time on my hands for these types of PRs. I gave it a go thinking it would take a few hours of work, but it ended up being a rabbit hole. I am calling it quits and leaving it up to people who actually want these things output. As I said, the issues are in the fragile (and hardcoded) unit tests and pam driver, so regular EAMxx cases/tests should work just fine with no additional work.

EAMxx: add some extra p3 diags

366edc6

this impl is kind of hacky because we don't yet have the facility to request output fields adhoc and these fields are buried deep in the p3 impl; in the future, this could be edited to be done more smartly, but for now, this may be just enough...

mahf708 added the EAMxx PRs focused on capabilities for EAMxx label Jan 24, 2025

mahf708 requested review from bartgol and jgfouca January 24, 2025 15:32

mahf708 changed the title ~~EAMxx: add some extra p3 diags~~ EAMxx: add p3 process rates as outputs Jan 24, 2025

mahf708 commented Jan 24, 2025

View reviewed changes

rljacob assigned bartgol Jan 24, 2025

jgfouca approved these changes Jan 24, 2025

View reviewed changes

bartgol approved these changes Jan 27, 2025

View reviewed changes

mahf708 commented Jan 27, 2025

View reviewed changes

mahf708 commented Jan 28, 2025

View reviewed changes

mahf708 closed this Jan 28, 2025

mahf708 mentioned this pull request Jan 28, 2025

EAMxx: add p3 rates diags #6936

Closed

mahf708 deleted the mahf708/eamxx/p3-extra-diags branch January 28, 2025 15:59

mahf708 mentioned this pull request Feb 6, 2025

EAMxx: Adds aerosols heterogeneous freezing calculations in P3 microphysics #6947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EAMxx: add p3 process rates as outputs #6938

EAMxx: add p3 process rates as outputs #6938

mahf708 commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

Built to branch `gh-pages` at 2025-01-24 15:34 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

mahf708 commented Jan 24, 2025

mahf708 Jan 24, 2025

jgfouca left a comment

bartgol left a comment

mahf708 Jan 27, 2025

bartgol Jan 27, 2025

mahf708 Jan 27, 2025

mahf708 Jan 28, 2025

bartgol Jan 28, 2025

mahf708 Jan 28, 2025

mahf708 Jan 28, 2025

mahf708 Jan 28, 2025

mahf708 Jan 28, 2025

mahf708 Jan 28, 2025

mahf708 commented Jan 28, 2025 •

edited

Loading

mahf708 commented Jan 28, 2025 •

edited

Loading

whannah1 commented Jan 28, 2025

mahf708 commented Jan 28, 2025

	static constexpr size_t NUM_ARRAYS = 47;
	static constexpr size_t NUM_ARRAYS = 34;

EAMxx: add p3 process rates as outputs #6938

EAMxx: add p3 process rates as outputs #6938

Conversation

mahf708 commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

Built to branch gh-pages at 2025-01-24 15:34 UTC. Preview will be ready when the GitHub Pages deployment is complete.

mahf708 commented Jan 24, 2025

Choose a reason for hiding this comment

jgfouca left a comment

Choose a reason for hiding this comment

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

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 commented Jan 28, 2025 • edited Loading

mahf708 commented Jan 28, 2025 • edited Loading

whannah1 commented Jan 28, 2025

mahf708 commented Jan 28, 2025

Built to branch `gh-pages` at 2025-01-24 15:34 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

mahf708 commented Jan 28, 2025 •

edited

Loading

mahf708 commented Jan 28, 2025 •

edited

Loading