updating unified MYNN-EDMF driver with packaged variables - #280
updating unified MYNN-EDMF driver with packaged variables#280joeolson42 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates MPAS atmosphere core plumbing to support a unified MYNN-EDMF driver integration by introducing new package flags for optional diagnostics (e.g., TKE budget and MF diagnostics), adjusting driver data movement accordingly, and updating build system paths to match the MYNN-EDMF repository’s new src/ layout.
Changes:
- Added new MYNN-EDMF packages in
Registry.xmlto separate TKE-budget and diagnostic-output variables for conditional allocation/output. - Updated PBL package setup and PBL driver code paths to use the new config/package gating and revised MYNN-EDMF driver argument list (including hydrostatic interface pressure).
- Updated Makefile/CMake source paths to reflect MYNN-EDMF code relocation into a
src/directory.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core_atmosphere/Registry.xml | Adds new MYNN-EDMF diagnostic packages and updates related namelist/variable package metadata. |
| src/core_atmosphere/physics/mpas_atmphys_packages.F | Activates new MYNN-EDMF packages based on config flags during physics package setup. |
| src/core_atmosphere/physics/mpas_atmphys_driver_pbl.F | Gates MYNN-EDMF diagnostic pool transfers on config flags and updates the MYNN-EDMF driver call signature. |
| src/core_atmosphere/physics/Makefile | Updates build recipe and include paths for the MYNN-EDMF src/ restructure. |
| src/core_atmosphere/CMakeLists.txt | Updates MYNN-EDMF source file paths to align with the new src/ layout. |
Suppressed comments (4)
src/core_atmosphere/physics/mpas_atmphys_driver_pbl.F:855
- Same as above: copying EDMF diagnostics from MPAS pools is currently gated on
config_mynn_edmf_output == 1, which will skip the copy for other nonzero modes described in Registry.xml.
if(config_mynn_edmf_output == 1)then
do j = jts,jte
do k = kts,kte
do i = its,ite
edmfa_p(i,k,j) = edmf_a(k,i)
edmfent_p(i,k,j) = edmf_ent(k,i)
edmfqc_p(i,k,j) = edmf_qc(k,i)
edmfqt_p(i,k,j) = edmf_qt(k,i)
edmfthl_p(i,k,j) = edmf_thl(k,i)
edmfw_p(i,k,j) = edmf_w(k,i)
subthl_p(i,k,j) = sub_thl(k,i)
subqv_p(i,k,j) = sub_qv(k,i)
detthl_p(i,k,j) = det_thl(k,i)
detqv_p(i,k,j) = det_qv(k,i)
enddo
enddo
enddo
endif
src/core_atmosphere/physics/mpas_atmphys_driver_pbl.F:1270
- Same gating issue when copying EDMF diagnostics back into MPAS pools:
config_mynn_edmf_output == 1will skip updates for other nonzero modes described in Registry.xml.
if(config_mynn_edmf_output == 1)then
do j = jts,jte
do k = kts,kte
do i = its,ite
edmf_a(k,i) = edmfa_p(i,k,j)
edmf_ent(k,i) = edmfent_p(i,k,j)
edmf_qc(k,i) = edmfqc_p(i,k,j)
edmf_qt(k,i) = edmfqt_p(i,k,j)
edmf_thl(k,i) = edmfthl_p(i,k,j)
edmf_w(k,i) = edmfw_p(i,k,j)
sub_thl(k,i) = subthl_p(i,k,j)
sub_qv(k,i) = subqv_p(i,k,j)
det_thl(k,i) = detthl_p(i,k,j)
det_qv(k,i) = detqv_p(i,k,j)
enddo
enddo
enddo
endif
src/core_atmosphere/physics/mpas_atmphys_packages.F:244
config_mynn_edmf_outputallows values 1 (updrafts) and 2 (downdrafts) per Registry.xml, but the package is only activated when the value is exactly 1. This prevents the output package from being enabled for the documented nonzero modes.
elseif(config_pbl_scheme == 'bl_mynnedmf') then
bl_mynnedmf_in = .true.
if(config_mynn_edmf_output ==1) bl_mynnedmf_output_in = .true.
if(config_mynn_tkebudget ==1) bl_mynnedmf_tkebudget_in = .true.
src/core_atmosphere/physics/mpas_atmphys_driver_pbl.F:1216
config_mynn_edmf_outputsupports nonzero modes beyond 1 per Registry.xml, but this guard only allocates/gets EDMF diagnostic arrays when the value is exactly 1. Use> 0so the driver can retrieve arrays for all enabled output modes.
if(config_mynn_edmf_output == 1) then
call mpas_pool_get_array(diag_physics,'edmf_a' ,edmf_a )
call mpas_pool_get_array(diag_physics,'edmf_ent' ,edmf_ent )
call mpas_pool_get_array(diag_physics,'edmf_qc' ,edmf_qc )
call mpas_pool_get_array(diag_physics,'edmf_qt' ,edmf_qt )
call mpas_pool_get_array(diag_physics,'edmf_thl' ,edmf_thl )
call mpas_pool_get_array(diag_physics,'edmf_w' ,edmf_w )
call mpas_pool_get_array(diag_physics,'sub_thl' ,sub_thl )
call mpas_pool_get_array(diag_physics,'sub_qv' ,sub_qv )
call mpas_pool_get_array(diag_physics,'det_thl' ,det_thl )
call mpas_pool_get_array(diag_physics,'det_qv' ,det_qv )
endif
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| call mpas_pool_get_array(diag_physics,'det_thl' ,det_thl ) | ||
| call mpas_pool_get_array(diag_physics,'det_qv' ,det_qv ) | ||
|
|
||
| if(config_mynn_edmf_output == 1) then |
There was a problem hiding this comment.
@joeolson42 since 1=updrafts and 2=downdrafts now, I presume Copilot's recommendation is reasonable since these variables would seem to apply for both updrafts and downdrafts?
There was a problem hiding this comment.
yes, this is a bug. Fixing with next commit.
| core_mynnedmf: core_physics_init core_physics_mmm | ||
| (cd physics_noaa/MYNN-EDMF; cp ./MPAS/Makefile .; cp ./MPAS/module_bl_mynnedmf_driver.F90 .; cp ./MPAS/module_bl_mynnedmf_common.F90 .; $(MAKE) all) | ||
| core_mynnedmf: core_physics_init | ||
| (cd physics_noaa/MYNN-EDMF/src; ln -s ./MPAS/Makefile; ln -s ./MPAS/module_bl_mynnedmf_common.F90; $(MAKE) all COREDEF="$(COREDEF)") |
There was a problem hiding this comment.
@joeolson42 what is your thinking for changing from cp to ln here, particularly when all of the other physics (granted, those we've added) are set up to use cp?
There was a problem hiding this comment.
I will take copilot's suggestion and use "ln -sf". I switched to linking from copying because I was sick of modifying the copied file for testing, then cleaning & recompiling (wiping out modified version), and then testing the old version. Commit fix in minutes.
| bl_mynn_in = .true. | ||
| elseif(config_pbl_scheme == 'bl_mynnedmf') then | ||
| bl_mynnedmf_in = .true. | ||
| if(config_mynn_edmf_output ==1) bl_mynnedmf_output_in = .true. | ||
| if(config_mynn_tkebudget ==1) bl_mynnedmf_tkebudget_in = .true. |
There was a problem hiding this comment.
This seasons reasonable to me, though I'm not sure if many/any of the other variables like this one have similar definitions.
There was a problem hiding this comment.
yes, this fix is also coming.
| call mpas_pool_get_config(configs,'config_mynn_mixqt' ,bl_mynn_mixqt ) | ||
| call mpas_pool_get_config(configs,'config_mynn_tkeadvect' ,bl_mynn_tkeadvect ) | ||
| call mpas_pool_get_config(configs,'config_mynn_tkebudget' ,bl_mynn_tkebudget ) | ||
| call mpas_pool_get_config(configs,'config_icloud_bl' ,icloud_bl ) | ||
| call mpas_pool_get_config(configs,'config_spp_pbl' ,spp_pbl ) |
There was a problem hiding this comment.
@joeolson42 if I'm interpreting this correctly, I think Laura's MYNN-EDMF implementation uses icloud_bl, such that entirely removing its definition breaks that implementation. However, the CI comparison tests all completed successfully (though the compile tests are failing at the GNU autotools stage -- which shouldn't be affected by this PR), so 🤷♂️
There was a problem hiding this comment.
icloud_bl is hard-coded to zero in mpas_atmphys_vars.F and it (and/or 'config_icloud_bl') does not exist as a namelist option in the Registry, so it needs to be removed. I think, in it's current form, this is a benign bug.
This PR brings in a unified (WRF/MPAS) driver with some advancements towards unification with CCPP. Additional changes include:
The testing was done in a commit prior to the stochastic physics, so the base already requires updating at our leisure.
Mandatory Questions
Reviews