Repository navigation
Conversation
1d8b5a4 to
38517b8
Compare
|
4f7920a to
24ce329
Compare
39c78a4 to
a57b412
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The implementation has a missing mam4xx API update, GPU-invalid host pointers, and incorrect dry-pressure units.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (4)
What changed in this PR
Couples ZM deep-convection outputs to MAM4xx convective wet scavenging.
Changes:
- Exposes ZM diagnostics required by MAM4xx.
- Adds configurable aerosol and gas convective processing.
- Adds physics-only, dynamics-physics, and CIME integration tests.
| File | Description |
|---|---|
components/eamxx/tests/multi-process/physics_only/mam/p3_zm_mam4_wetscav/output.yaml |
Defines test outputs. |
components/eamxx/tests/multi-process/physics_only/mam/p3_zm_mam4_wetscav/input.yaml |
Configures the physics-only test. |
components/eamxx/tests/multi-process/physics_only/mam/p3_zm_mam4_wetscav/CMakeLists.txt |
Registers the test. |
components/eamxx/tests/multi-process/physics_only/CMakeLists.txt |
Enables the test directory. |
components/eamxx/tests/multi-process/dynamics_physics/mam/homme_shoc_cld_spa_p3_rrtmgp_zm_mam4_wetscav/output.yaml |
Defines coupled test outputs. |
components/eamxx/tests/multi-process/dynamics_physics/mam/homme_shoc_cld_spa_p3_rrtmgp_zm_mam4_wetscav/input.yaml |
Configures the coupled process chain. |
components/eamxx/tests/multi-process/dynamics_physics/mam/homme_shoc_cld_spa_p3_rrtmgp_zm_mam4_wetscav/CMakeLists.txt |
Registers the coupled test. |
components/eamxx/tests/multi-process/dynamics_physics/mam/CMakeLists.txt |
Enables the coupled test directory. |
components/eamxx/src/physics/zm/impl/zm_conv_main_impl.hpp |
Initializes inactive-column cloud-top output. |
components/eamxx/src/physics/zm/eamxx_zm_process_interface.cpp |
Exports ZM coupling fields. |
components/eamxx/src/physics/mam/eamxx_mam_wetscav_process_interface.hpp |
Adds ConvProc state and workspace. |
components/eamxx/src/physics/mam/eamxx_mam_wetscav_process_interface.cpp |
Integrates ConvProc with ZM outputs. |
components/eamxx/cime_config/testdefs/testmods_dirs/eamxx/mam4xx/zm/shell_commands |
Configures the CIME ZM/MAM4xx test. |
components/eamxx/cime_config/namelist_defaults_eamxx.xml |
Adds ConvProc defaults. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f309386 to
a5bb2ce
Compare
|
As we discussed yesterday, it would be a good idea to consolidate the convective vertical transport in one place. But I think that can wait for a follow-up PR. |
To be clear - "consolidate" does not mean we remove the ZM tracer transport since we discussed scenarios where we might want to use that capability. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Remember that there is a potential bug in |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| add_field<Computed>("zm_dcape", scalar2d, J/kg/s, grid_name); | ||
| add_field<Computed>("zm_activity", scalar2d, none, grid_name); | ||
| add_field<Computed>("zm_jt", scalar2d, none, grid_name); | ||
| add_field<Computed>("zm_jcbot", scalar2d, none, grid_name); |
There was a problem hiding this comment.
ZM scheme uses the parcel lifting level (maxi) as the cloud-base level, which is appropriate for convective transport. However, for aerosol activation, the lifting condensation level (LCL) may be more appropriate, as it represents the level at which a rising air parcel becomes saturated, allowing aerosol activation and cloud droplet formation to occur.
There was a problem hiding this comment.
@DJFJJA could you please point to the variable name (Fortran) of the lifting condensation level (LCL)?
There was a problem hiding this comment.
@odiazib the variable jlcl in zm_conv_main() gives the index of the LCL.
There was a problem hiding this comment.
maxi represents the parcel lifting level, corresponding to jcbot (the cloud-base level index), while jctop represents the cloud-top level index.
There was a problem hiding this comment.
So, zm tracer transport uses maxi (is this jctop?). But we want the aerosol tracer to use jlcl instead of jctop? Note: for gases, which we agree will be handled by MAM4xx, we still need to use jctop. @DJFJJA and @whannah1.
To clarify - maxi was the old name for the level of maximum MSE used to launch parcels, and maxg was the "gathered" version. In the fortran refactor I renamed these to be msemax_klev / msemax_klev_g.
@DJFJJA was pointing out that the LCL may be "above" jcbot and the layers between the parcel launch level and the LCL might not have any condensate, thus no aerosol activation should happen in those levels. Whereas it might still be appropriate to use the parcel launch level for transport. I think it's a good point, but I'm not sure how to address it.
There was a problem hiding this comment.
@kaizhangpnl and @DJFJJA, can we handle this bug in a follow-up PR? This PR and its associated MAM4xx PR are too large. Note that I created an issue for this bug here: E3SM-Project/mam4xx#612
…ld_kbot and zm_jctop -> zm_cld_ktop.
8c82a19 to
5986c44
Compare
|
Many thanks for all your work in finalizing this PR. I have one additional comment: we should verify that the units of all fields passed to Two potential inconsistencies:
Could you help confirm that the necessary conversions are applied and check the remaining input fields for similar inconsistencies? Thanks! |
…of zm_cld_bot for kbot.
@kaizhangpnl The units of kg/m²/s come from the ZM interface see here. @whannah1 Is there a typo in the units of |
@kaizhangpnl I corrected the units for dpdry. Note that dpdry is used only by ma_convproc_intr. |
In the ZM scheme, the updraft mass flux (mflx_up) is calculated in mb/s. In the Fortran implementation, it is converted to kg/m²/s using hydrostatic balance, with the conversion factor 100/g, before being stored in the output diagnostic arrays. For example, in zm_conv_intr.F90: However, I couldn't find an equivalent unit conversion in the current C++ implementation. In eamxx_zm_process_interface.cpp, zm_mflx_up is registered with units of kg/m²/s, but the output appears to be copied directly from the internal mflx_up array, which is documented as mb/s. @whannah1 Could you double-check this? |
|
I am marking this PR as ready for review so the tests can run on CI. |
| rliq(i) = 0; | ||
| dsubcld(i) = 0; | ||
| jctop(i) = pver - 1; | ||
| jctop(i) = -1; |



The ZM+MAM4xx coupling is based on E3SM v2 and currently includes only the MAM4xx side. Please check for any missing code integration. I will keep this PR as a draft, as there is still a lot of work and discussion to be done.
see also: E3SM-Project/mam4xx#587
This PR is BFB. However, we added two multi-process tests, one of which checks baselines. Since baselines have not been created for this test yet, it will fail:
homme_shoc_cld_spa_p3_rrtmgp_zm_mam4_wetscav_baseline_cmp
Failed: baseline_cmp, baseline_gen