EAMxx: add ability to use separate cloud fractions in p3

[hassanbeydoun]

This PR is an attempt at allowing P3 to see separate liquid and ice fractions. Code changes involve passing cldfrac_liq and cldfrac_ice to P3 (instead of cldfrac_tot) and modifying the WBF process based on work done by Lin Lin in THREAD.

[BFB]

[github-actions]

PR Preview Action v1.6.0
🚀 View preview at https://E3SM-Project.github.io/E3SM/pr-preview/pr-6966/
Built to branch gh-pages at 2025-02-04 18:16 UTC. Preview will be ready when the GitHub Pages deployment is complete.

[mahf708]

@hassanbeydoun please review this file carefully from line 10 to line 69. Is that what you want here? If so, we can begin to integrate this PR after a few tests.

[mahf708]

I invited people who touched nearby code to review. The non-default option use_separate_ice_liq_frac allows one to use speciated subgrid cloud fractions coming from shoc. It's likely okay to use, but it is not default for a reason. YMMV. Hassan and co will evaluate it.

From a code standpoint, there are two shady hacks in the code. One is annotated with a FIXME; the other is passing runtime_option struct as a pointer to get away with not giving it in tests. Feel free to call them out, but please suggest alternate impl :) a third hack is overloading the cloud_water_conservation function... I hate all these hacks, but I also don't want to open the can of worms that's chasing these things everywhere. In a future rewrite of P3, we will tidy it up everything

[bartgol]

Since Naser brought it up... I think this is fine, but a couple of alternatives for you in case you find this underwhelming.

1. Blow, you can be more compact and do

bool use_separate_ice_liq_frac = runtime_options and runtime_options->use_separate_ice_liq_frac;

Booleans are short-circuited in C++, so if runtime_options==nullptr, the 2nd part of the and is not evaluated (and you don't deref the null ptr).
2. You could pass runtime as a ref, and default to = {}. If the struct inits all param to the same defaults you use in the tests, you're good. E.g., if it has

struct P3Runtime {
...
  bool use_separate_ice_liq_frac = false;
};

having const P3Runtime runtime_options = {} in the fcn signature will make you have use_separate_ice_liq_frac = false by default.

[mahf708]

improved in 46c43ed

[jgfouca]

Since I don't see any changes in p3/tests, I assume no test for this second cloud_water_convservation impl was added. Should we add one?

[jgfouca]

Strange indentation here.

[mahf708]

The indentation was a silly attempt at keeping things like above:

  const  auto& cld_frac_t_in  = get_field_in("cldfrac_tot").get_view<const Pack**>();
  // FIXME: This is hack to get things going, these fields should be strictly const
  // But to get around bfb testing and needing to declare these fields elsewhere,
  // We will just use this workaround ...
         auto  cld_frac_l_in  = cld_frac_t_in;

will let a linter fix it in the next commit

[mahf708]

fixed in 43bf1a4

[jgfouca]

Some minor code formatting stuff. 4-space indents are being used here instead of the standard 2. Also, many of the if and else statements are not formatted like the rest of eamxx. They should be
if<space>(<condition>)<space>{
and
else<space>{

[mahf708]

This is actually identical to the one below (the other impl, the og)

Here's the diff between the two:

diff --git a/og b/ng
index 48b8efd..e5e6090 100644
--- a/og
+++ b/ng
@@ -4,7 +4,7 @@ void Functions<S,D>
 ::cloud_water_conservation(const Spack& qc, const Scalar dt,
   Spack& qc2qr_autoconv_tend, Spack& qc2qr_accret_tend, Spack &qc2qi_collect_tend, Spack& qc2qi_hetero_freeze_tend, 
   Spack& qc2qr_ice_shed_tend, Spack& qc2qi_berg_tend, Spack& qi2qv_sublim_tend, Spack& qv2qi_vapdep_tend,
-  Spack& qcheti_cnt, Spack& qicnt, const bool& use_hetfrz_classnuc, const Smask& context)
+  Spack& qcheti_cnt, Spack& qicnt, const bool& use_hetfrz_classnuc, const Spack& cld_frac_l, const Spack& cld_frac_i, const Smask& context)
 {
 
   Spack sinks;
@@ -15,6 +15,7 @@ void Functions<S,D>
     sinks = (qc2qr_autoconv_tend+qc2qr_accret_tend+qc2qi_collect_tend+qc2qi_hetero_freeze_tend+qc2qr_ice_shed_tend+qc2qi_berg_tend)*dt; // Sinks of cloud water
   }
   const auto sources = qc; // Source of cloud water
+  const auto il_cldm = min(cld_frac_i,cld_frac_l); 
   Spack ratio;
 
   constexpr Scalar qtendsmall = C::QTENDSMALL;
@@ -45,9 +46,14 @@ void Functions<S,D>
   //"ratio" of timestep and vapor deposition and sublimation  for the
   //remaining frac of the timestep.  Only limit if there will be cloud
   // water to begin with.
+  // in instances where ratio < 1 and we have qc, qidep needs to take over
+  // but this is an in addition to the qidep we computed outside the mixed
+  // phase cloud. qidep*(1._rtype-ratio)*(il_cldm/cld_frac_i) is the additional
+  // vapor depositional growth rate that takes place within the mixed phase cloud
+  // after qc is depleted
   enforce_conservation = sources > qtendsmall && context;
   if (enforce_conservation.any()){
-    qv2qi_vapdep_tend.set(enforce_conservation, qv2qi_vapdep_tend*(1-ratio));
+    qv2qi_vapdep_tend.set(enforce_conservation, qv2qi_vapdep_tend + qv2qi_vapdep_tend*(1-ratio)*(il_cldm/(cld_frac_i-il_cldm)));
     qi2qv_sublim_tend.set(enforce_conservation, qi2qv_sublim_tend*(1-ratio));
   }
 }

we haven't adopted a specific style guideline yet. The default LLVM c++ style does call for 2 spaces indent and no-space after if/else

[singhbalwinder]

Since this function is overloaded, it leads to code duplication. To avoid that, we may need to restructure the logic (either using helper functions with core functionality or using optional arguments). I am not sure how much effort that is or if it should be done in this PR.

[mahf708]

I got rid of the overloading in 6cebbcf

[singhbalwinder]

Nice work! I just left a minor comment.

[singhbalwinder]

Since this function is overloaded, it leads to code duplication. To avoid that, we may need to restructure the logic (either using helper functions with core functionality or using optional arguments). I am not sure how much effort that is or if it should be done in this PR.

[jgfouca]

@mahf708 , thanks, I withdraw my comments about the formatting. We still have no unit test for the new impl. If you think that's not a concern, then I'm fine with it.

[mahf708]

thanks, I withdraw my comments about the formatting. We still have no unit test for the new impl. If you think that's not a concern, then I'm fine with it.

the formatting comment is welcome anytime :) I'm happy to format however desired; I was just pointing out I was just copying stuff around

re unit tests, yep, we are only testing the default config (which is what's officially supported). I would like to add unit tests for non-default options BUT we are not yet sure the non-default formulation is actually scientifically reasonable. Once it is verified and we feel more comfortable with it, and before it is supported officially, I will add unit tests for it. I anticipate once the science people run a few tests, they will likely uncover scientific issues that may need to be corrected (I'm not actively testing this new option)

[AaronDonahue]

Is this ready to be merged?

[mahf708]

Is this ready to be merged?

Yes, feature is inactive, so should be good to go