Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implementation of MYNN-EDMF submodule #2148

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Implementation of MYNN-EDMF submodule #2148

wants to merge 5 commits into from

Conversation

joeolson42
Copy link
Contributor

This replaces the original (non submodule) MYNN with the public-facing MYNN-EDMF submodule, which will the central point of development for the MYNN-EDMF in WRF, MPAS, and CCPP.

TYPE: bug fix, enhancement, and new feature

KEYWORDS: refactored, submodule, MYNN-EDMF

SOURCE: Joseph Olson (NOAA/GSL)

DESCRIPTION OF CHANGES:

  1. Submodule implementation pointing to https://github.com/NCAR/MYNN-EDMF/
  2. This is a refactored scheme (now k-only) which is called from a new driver derived heavily from the MPAS driver.
  3. The module names changed from mynn to mynnedmf to resolve a version conflict in MPAS.
  4. This version carries many modification that originated in FV3-related development for RRFSv1, but it has since been tuned to better perform in MPAS and WRF, unlike previous versions of the MYNN. The list of modifications are fairly extensive but the most dycore-sensitive parts of the MYNN are related to the subgrid clouds. From now on, all minor commits are captured in the public-facing submodule repository and the submodule will be updated in WRF at a lower frequency (probably every 6 months or so).

LIST OF MODIFIED FILES: list of changed files (use git diff --name-status master to get formatted list)
M .gitmodules
M Makefile
M Registry/Registry.EM_COMMON
M clean
M dyn_em/module_first_rk_step_part1.F
M main/depend.common
A phys/MYNN-EDMF
M phys/Makefile
D phys/module_bl_mynn.F
D phys/module_bl_mynn_common.F
D phys/module_bl_mynn_wrapper.F
M phys/module_pbl_driver.F
M phys/module_physics_init.F

TESTS CONDUCTED:

  1. The submodule itself has been tested in both WRF and MPAS for several months now in NOAA-GSL realtime systems and case study tests, passing all of our newly designed regression tests.
  2. Has been compiled and run with intel and gnu compilers on Jet and Hera.
  3. Are the Jenkins tests all passing?

RELEASE NOTE: Submodule implementation of the MYNN-EDMF (https://github.com/NCAR/MYNN-EDMF). The module names changed from mynn to mynnedmf to resolve a version conflict in MPAS. This version was originally developed within FV3/CCPP for RRFSv1, but has been refactored (to a k-only scheme) resulting in a speed-up of about 10-15% and it has since been tuned to better perform in MPAS and WRF compared to previous versions which were primarily developed for use in FV3.

@joeolson42 joeolson42 requested review from a team as code owners December 31, 2024 21:16
@joeolson42
Copy link
Contributor Author 

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

@joeolson42
Copy link
Contributor Author

Results after the latest update:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

dudhia
dudhia approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@dudhia dudhia left a comment

Choose a reason for hiding this comment

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

Fine from rest-of-physics perspective

weiwangncar
weiwangncar reviewed Jan 13, 2025
View reviewed changes
phys/Makefile
@if [ \( ! -f module_bl_mynnedmf.F \) -o \( ! -f module_bl_mynedmf_common.F \) -o \
\( ! -f module_bl_mynnedmf_driver.F \) ] ; then \
echo Pulling in MYNN-EDMF submodule ; \
( cd .. ; git submodule update --init --recursive ) ; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this always get the latest modules/files from the MYNN repository?

@joeolson42
Copy link
Contributor Author

joeolson42 commented Jan 13, 2025 via email

@dudhia
Copy link
Collaborator

dudhia commented Jan 13, 2025

OK. That's the behavior we want. We want the hash to stay fixed for a release.

islas
islas requested changes Jan 14, 2025
View reviewed changes
Copy link
Collaborator

@islas islas left a comment

Choose a reason for hiding this comment

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

Is there any specific reason we want to continue using git submodules for new changes, as opposed to manage_externals?

Also this needs to be updated for the CMake build as well.

@dudhia
Copy link
Collaborator

dudhia commented Jan 14, 2025 via email

@islas
Copy link
Collaborator

islas commented Jan 14, 2025

@dudhia I understand, however as we now have manage_externals in place within the develop branch, if we plan on managing submodules that way we have the opportunity to implement these changes with manage_externals right now while this is still in review. Deferring this adds technical debt that must be address by someone at some future time.

If our plans are not to move things under manage_externals, then that's fine but it does beg the question why manage_externals was introduced in the first place...

@dudhia
Copy link
Collaborator

dudhia commented Jan 14, 2025 via email

@weiwangncar
Copy link
Collaborator

My understanding is that whether using manage externals or submodule does not have impact on how the source code repositories are maintained. It is the choice of the host model.
Since Joe is not familiar with the use of manage externals, one way to work with this is to merge this PR first, and I can make the change to use manage externals after that.

@dudhia
Copy link
Collaborator

dudhia commented Jan 15, 2025 via email

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

@islas islas islas requested changes

@dudhia dudhia dudhia approved these changes

@weiwangncar weiwangncar weiwangncar approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Develop Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joeolson42 @dudhia @islas @weiwangncar