EAMxx: Output SO4 and H2SO4 deposition flux diagnostics

[overfelt]

Add the diagnostic output fields named: "dqdt_so4_aqueous_chemistry" "dqdt_h2so4_uptake"

These are intermediate values computed during the MAM4xx atmospheric chemistry and microphysics calculation. MAM4 F90 had them as output diagnostics, and this PR just enables the same for MAM4xx.

This PR brings in all the latest changes in the MAM4xx submodule, which includes an NBFB change (see MAM4xx PR #447)

[NBFB] because of the changes in the MAM4xx submodule (not related to the diagnostics)

[singhbalwinder]

This PR will likely be NBFB, not because of diagnostics, but because of an update to the MAM4xx submodule. MAM4xx submodule has NBFB changes.

[overfelt]

Can the mam4xx submodule be updated in two steps? One for the non-BFB change and one for this change which is BFB?

[singhbalwinder]

I will start merging this PR once the testing concludes.

[odiazib]

@mjschmidt271 @singhbalwinder Should this eamxx-format / clang-format-linter (pull request) be handled?

[overfelt]

The problem with clang-formatting a simple change is that the clang formatting overwhelms all the other changes making it impossible to see the relevant changes. A separate clang-format only commit is needed.

[singhbalwinder]

Yeah, after we merge this PR, we can reformat all MAM4xx interfaces and the other files we touched (as we did in the past) in a separate PR.

[mjschmidt271]

@mjschmidt271 @singhbalwinder Should this eamxx-format / clang-format-linter (pull request) be handled?

@odiazib I think Balwinder's suggestion sounds like the best bet at this point.

Enforcement is a little tricky at the beginning, but once we're through the major format changes it'll be more consistently enforced.

[mahf708]

I think you should make optional diagnostics optional in a way that doesn't increase memory. Here, you could simply hide away the diagnostics behind an extra_diags flag

  // ... 

+ if (extra_diags) {
  add_field<Computed>("dqdt_so4_aqueous_chemistry", vector2d_nmodes, kg/m2/s,  grid_name);
  add_field<Computed>("dqdt_h2so4_uptake", vector2d_nmodes, kg/m2/s,  grid_name);
+ }

  // ...
  
+ if (extra_diags) {
  // Constituent fluxes 
  view_2d aqso4_flx = get_field_out("dqdt_so4_aqueous_chemistry").get_view<Real **>();
  view_2d aqh2so4_flx = get_field_out("dqdt_h2so4_uptake").get_view<Real **>();
+ } else {
+ view_2d aqso4_flx;
+ view_2d aqh2so4_flx;
+ }

  // ...
+         if (extra_diags) {
          Kokkos::parallel_for(Kokkos::TeamVectorRange(team, nmodes), [&](int m) {
           aqso4_flx(icol, m) = aqso4_flx_col[m];
           aqh2so4_flx(icol, m) = aqh2so4_flx_col[m];
          });
+         }
 // ...

that will help with perf and general tidiness, but it is ofc up to you how to proceed.

Also, ideally, the vertical reduction should be done here, not in MAM. So, the 3D version of these values should be exposed, not the already reduced (2D). People will likely request the 3D version at some points, which will require another PR and another set of changes etc

[singhbalwinder]

Thanks, Naser — those are great suggestions! @overfelt also mentioned the idea of adding a flag to hide the diagnostics, which will help with memory. I agree that exposing the 3D fields could be a better approach than the current one. I’ll let @kaizhangpnl share his thoughts on the 3D fields too.

[kaizhangpnl]

Thanks, Naser — those are great suggestions! @overfelt also mentioned the idea of adding a flag to hide the diagnostics, which will help with memory. I agree that exposing the 3D fields could be a better approach than the current one. I’ll let @kaizhangpnl share his thoughts on the 3D fields too.

It would be nice to have the optional 3D fields.

[singhbalwinder]

The tests are passing, so I am planning to merge this PR.

There are some outstanding issues (e.g., hide diagnostics using a namelist flag), and we will address those in a small follow-up PR.

[mahf708]

The tests are passing, so I am planning to merge this PR.

There are some outstanding issues (e.g., hide diagnostics using a namelist flag), and we will address those in a small follow-up PR.

Why not expose 3D fields now?

[singhbalwinder]

Why not expose 3D fields now?

This PR has a lot of other changes from the MAM4xx submodule and the HAERO submodule (e.g., cmake changes). We would like to merge this PR as the tests are passing now (on Frontier and other machines), and we can merge these large changes. We will expose 3d fields and add a namelist variable in the upcoming PRs with small changes. Keeping those changes separate allows us to better isolate issues if anything breaks. We already have several small, focused PRs lined up to address these.

[mahf708]

Why not expose 3D fields now?

This PR has a lot of other changes from the MAM4xx submodule and the HAERO submodule (e.g., cmake changes). We would like to merge this PR as the tests are passing now (on Frontier and other machines), and we can merge these large changes. We will expose 3d fields and add a namelist variable in the upcoming PRs with small changes. Keeping those changes separate allows us to better isolate issues if anything breaks. We already have several small, focused PRs lined up to address these.

Makes little sense to me tbh. If you want to keep things separate, then don't change anything in interfaces (like the add_field calls). You can also do a clean submod update separately in another pr and then rebase this pr after that one is merged. This way you're rushing a half-baked feature because "tests are passing" which doesn't seem sound to me

[singhbalwinder]

We have changed the signatures of some routines in MAM4xx that interface with EAMxx. So, a "clean" submodule update is not possible unless we change the submodule. The other option is to keep the submodule the same and add minimal changes to the EAMxx interface. There may be other ways to do it piecewise, but all will incur additional work.

I don't think these are half-baked features. It provides what the eval team needs at this time. These changes also work on the machines that we are supporting for the eval team. Therefore, merging this PR will help with that. Exposing the 3D fields is something extra that we can do, and we plan to do it in a smaller PR. Similarly, adding a namelist will be part of another PR that may also package these diagnostics in a clean struct.

We would like to bring additional changes in small PRs, as it is easy to review, test, and merge faster. As a reviewer, if you think it makes more sense to add those in this PR, we can do that as well.

[singhbalwinder]

I am planning to merge this PR and start a new PR to address the following:

1. Expose the 3D fields
2. Fix diagnostics names in the EAMxx interface
3. Add a guard for these diagnostics so that they are turned on via a namelist flag
4. Clean up the calculation of the 2D field in MAM4xx if we can deduce it from the 3D fields using the utility functions.