Skip to content

Conversation

@kuanchihwang
Copy link
Collaborator

Originator(s):

kuanchihwang

Descriptions (include the issue title, and the keyword ['closes', 'fixes', 'resolves'] followed by the issue number):

This PR adds the Mellor-Yamada-Nakanishi-Niino (MYNN) surface layer scheme to the experimental convection-permitting physics suite.

List all namelist files that were added or changed:

None

List all files eliminated and why:

None

List all files added and what they do:

A       .gitattributes
  * Make GitHub Linguist recognize `*.pf` files as Fortran source code
A       schemes/mmm/sf_mynn_compat.F90
A       schemes/mmm/sf_mynn_compat.meta
  * Implement MYNN surface layer scheme

List all existing files that have been modified, and describe the changes:

M       .github/workflows/unit-tests.yaml
  * Do not hard code path to working directory in GitHub Actions
M       .gitmodules
M       schemes/mmm/mmm_physics
  * Update git submodule of MMM physics
M       schemes/mmm/CMakeLists.txt
M       test/unit-test/tests/mmm/CMakeLists.txt
  * Synchronize `CMakeLists.txt` of MMM physics with MPAS dycore in CAM-SIMA
M       schemes/mmm/cu_ntiedtke_compat.F90
M       schemes/mmm/cu_ntiedtke_compat.meta
  * Fix incorrect standard name in new Tiedtke convection scheme
M       schemes/mmm/mmm_physics_compat.F90
M       schemes/mmm/mmm_physics_compat.meta
  * Implement interstitial schemes for MYNN surface layer scheme
M       test/test_suites/suite_convection_permitting.xml
  * Add MYNN surface layer scheme to convection-permitting suite
M       test/unit-test/tests/mmm/mmm_physics_compat_tests.pf
  * Implement unit tests

List all automated tests that failed, as well as an explanation for why they were not fixed:

None

Is this an answer-changing PR? If so, is it a new physics package, algorithm change, tuning change, etc?

Answer-changing for the convection-permitting physics suite due to a newly added physics scheme. Nothing is changed for the rest.

If yes to the above question, describe how this code was validated with the new/modified features:

The convection-permitting physics suite is considered an experimental feature. There is no baseline available to validate against because it has never been implemented in CAM-SIMA as well as CAM before.

@kuanchihwang kuanchihwang marked this pull request as ready for review December 10, 2025 00:05
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing in the first MYNN scheme @kuanchihwang! I do have some change requests, mostly related to the location of certain subroutines, the conversion of hard-coded variables to namelist variables, and a few other general requests. Of course if you have any questions or concerns with my requests just let me know. Thanks again!

Comment on lines +296 to +301
[ spp_pbl ]
standard_name = flag_for_stochastically_perturbed_parameterization_for_mynn_scheme
units = flag
type = logical
dimensions = ()
intent = out
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to turn this into a namelist variable so that a user can configure it at runtime? All you would need to do is add a namelist XML file which contains the relevant variable and default value(s). You can find documentation describing the needed XML file here:

https://escomp.github.io/CAM-SIMA-docs/conversion/create-namelist-xml/

And various examples throughout ESCOMP/atmospheric_physics (just search for XML files under any of the directories under schemes).

Finally, if you do bring in this file, then you can remove the variable entirely from sf_mynn_compat_pre_run as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In MPAS, the spp_pbl option is hard-coded to 0 (.false.), and is not configurable by users.

spp_pbl starts out as an integer parameter having a value of 0[1]. It is supplied as an argument to the first layer physics driver of MYNN surface layer scheme[2]. In the second layer, the logical variable f_spp is set to .false. because spp_pbl is 0[3]. And also notice that the optional argument pattern_spp_pbl in [3] is not even supplied from [2]. This optional argument eventually becomes the array rstoch1d, which is reason that it is filled with zeros here indicating no randomness. Finally, a few blocks below, f_spp is supplied as the named argument spp_pbl to the CCPP-ized scheme sf_mynn_run[4], which is what we do here.

In all, this option has never been configurable in MPAS, and I am not sure if other choices have ever been tested before. Please let me know if you would still like this to be configurable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the background! I agree that it looks like it has never been configurable in MPAS-A (at least outside of expert users), so I am happy to keep it out of the namelist here for now.

However, given this I might vote to just make it a module-level parameter, similar to sea_ice_area_fraction_threshold, and just pass that parameter directly into sf_mynn_run. That will help clean-up the code in the CAM-SIMA CCPP layer, and make it more clear to users/developers that this parameter shouldn't generally be modified.


<scheme>mmm_physics_compat</scheme>

<scheme>sf_mynn_compat_pre</scheme>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this suite is getting bigger, can we add comments to the set of scheme calls for each parameterization? For example:

Suggested change
<scheme>sf_mynn_compat_pre</scheme>
<!-- Mellor–Yamada–Nakanishi–Niino (MYNN) Surface Layer -->
<scheme>sf_mynn_compat_pre</scheme>

You can also see examples of these comments in the CAM4 suite as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments to the suite definition file in 9e86e51.

However, this standard name change deviates from the ESM Standard Name Library.

In the ESM Standard Name Library, "ratio_of_height_to_monin_obukhov_length" is
defined. However, here "ratio_of_height_at_surface_adjacent_layer_to_monin_obukhov_length"
is used.
…ters

* To conform to the existing naming convention in state converters,
  `compute_ ...` has been renamed to `calc_ ...`.
* Fix incorrect units in a code comment in state converters.
@nusbaume nusbaume requested a review from cacraigucar January 23, 2026 17:02
path = schemes/mmm/mmm_physics
url = https://github.com/NCAR/MMM-physics.git
fxtag = 20250616-MPASv8.3
fxtag = 6cb29bc8f17bc3efdfaaa8a268ca53e2504aadbd
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to request that this be a tag and not hash. There are several reasons:

  • This repo is a part of the CAM/CESM release. In those repos, we request tags for every external
  • When a user sees a hash, they have no way to determine if this version is 3 years old or more recent than say the 2025-616-MPASv8.3 tag which they are familiar with.

Comment on lines +48 to +50
! This threshold is hardcoded to the same value as in MMM physics.
! It is named `xice_threshold` there.
real(kind_phys), parameter :: sea_ice_area_fraction_threshold = 0.02_kind_phys
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the comment indicates this is hardcoded to match the value in MMM physics, is it possible to somehow get xice_threshold into this routine? This would eliminate the possibility of it being changed in one location or not the other. Alternatively, could a check be performed somewhere during initialization to make sure they match? Perhaps a check_mmm_constanst routine whose scope is such that it has access to these routines as well as the MMM library ones and performs a one-time check.

Comment on lines +15 to +17
! This threshold is hardcoded to the same value as in MMM physics.
! It is named `xice_threshold` there.
real(kind_phys), parameter :: sea_ice_area_fraction_threshold = 0.02_kind_phys
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I now see that this variable is being set in at least three locations, it is important to have it defined in one location or at least set in one location on this interface side and compared with the value in the MMM physics.

Comment on lines +249 to +253
allocate(mask_sea_ice_cell(ncol), errmsg=errmsg, stat=errflg)

if (errflg /= 0) then
return
end if
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that the errmsg coming back from allocate is generic (I don't know for certain). It might be good to add a line like errmsg = "sf_mynn_compat_run; Variable: mask_sea_ice_cell; " //errmsg

Comment on lines +18 to +21
! Should set options that are specific to MMM physics.
@assertEqual(1, isfflx)
@assertEqual(0, isftcflx)
@assertEqual(0, iz0tlnd)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the MMM physics values were to ever change, this test would not catch that. Is it possible to have this test access the values in MMM physics instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants