Implement finalization and various enhancements for MPAS dynamical core #327
Conversation
Additional comments:Note to reviewers: It might be easier to review the PR commit by commit. The commit messages provide the context and rationale behind each set of changes. You should be able to run analytic cases currently supported by CAM-SIMA (e.g., You are also encouraged to test MPAS dynamical core in single precision mode. From the following benchmarks, MPAS dynamical core in single precision mode is ~40% faster and uses ~25% less memory than in double precision mode. A reference CAM run with the same configuration is also provided for completeness.
|
kuanchihwang
force-pushed
the
staging/implement-dyn-final
branch
3 times, most recently
from
cd464ea to
90fc72a
Compare
* Adjust wording and keep code comments up-to-date * Concentrate all calls to `mark_as_initialized` in one place * Fix up code style inconsistencies
Also fix variable rank information.
Relax restrictions on mixed precision between MPAS dynamical core and its input data.
`get_variable_pointer` also works for accessing MPAS configuration. It has built-in error checking so no need to do it manually.
Comply with CAM(-SIMA) coding standards. However, kind and length parameters are exempt from this rule. They are better at module level.
It is now possible to run CAM-SIMA with MPAS dynamical core in single precision mode. It can be enabled by defining the `SINGLE_PRECISION` macro in `CPPFLAGS`.
Protect these variables from being modified externally.
Manipulating source code by running `sed` commands during building is neither elegant nor obvious. Downstream modifications like these are better achieved and communicated through patches. This is also the standard practice in open source software.
These patches are applied before building.
This Python script generates the XML namelist definition file of MPAS dynamical core from MPAS registry. Optionally, if an XML schema is provided, the generated file can also be validated for correctness.
From now on, it can be generated automatically from MPAS registry.
Add MPAS initial files for running analytic cases.
kuanchihwang
force-pushed
the
staging/implement-dyn-final
branch
from
90fc72a to
e36fcdb
Compare
There was a problem hiding this comment.
Looks great @kuanchihwang! I just have a few questions and some very minor requests, mostly to move some remaining module-level use statements to their respective subroutines. Of course if any of my requests are problematic just let me know. Thanks!
| @for file in *.patch; do \ | ||
| if git apply --check -p2 "$${file}"; then \ | ||
| echo "Applying $${file}"; \ | ||
| git apply -p2 "$${file}"; \ | ||
| fi; \ |
There was a problem hiding this comment.
Do you have any sense what the medium or long-term maintainability of this method will be? For example, will we need to create new patch files every time we update the MPAS submodule in CAM-SIMA, or are the targeted files rarely ever modified?
I don't think it matters for this PR, but if you think that it could require an abnormal amount of work to maintain then it might be worth opening a Github issue so we can remember to try and figure out alternative methods in the future.
There was a problem hiding this comment.
I'm also wondering whether we might be incurring greater maintenance work with the patch file approach. For example, I'm currently working on some changes in the MPAS registry tool's gen_inc.c code, and I suspect that after those changes make their way into the MPAS-A develop branch, we'd need to create a new patch file for that same file in CAM-SIMA during the next update of the MPAS-A dycore.
@kuanchihwang I'd be glad to work with you on incorporating generalizations upstream in the MPAS-Dev/MPAS-Model repository to support CAM-SIMA -- for example, for modifying the prefix of namelist options.
There was a problem hiding this comment.
For 0001-Prefix-all-MPAS-namelist-group-and-option-names.patch, I will work with @mgduda to integrate the namelist transformation logic natively into the code generation utility of MPAS, so we no longer have to patch it here in the future.
I think both 0002-Disable-physics-for-MPAS-dycore-only-build.patch and 0003-Declare-constants-at-the-native-precision-of-MPAS.patch are fairly straightforward to upstream. I will open their respective PRs at the MPAS repository, so we can also drop those patches in the future.
Finally, for the patching mechanism, it is a common practice in open source software. Take Python 3.12 for example, Debian Linux maintains ~35 downstream patches, while Rocky Linux, an RHEL clone, also has ~10. When the upstream project gets updated, the patches will get rebased and refreshed as needed. But I agree, we definitely should keep the number of downstream patches as few as possible.
There was a problem hiding this comment.
This sounds like a good plan to me! Please let me know if there is anything that I can do to help, especially on the CAM-SIMA-side.
There was a problem hiding this comment.
This sounds fine to me as well. We can create PRs to CAM-SIMA to remove the need for specific patches once the upstream MPAS-Model code has suitable modifications in place.
... Also for kind and length parameters.
|
@kuanchihwang I recognize that it's taken me an awfully long time to review CAM-SIMA PRs related to MPAS in the past, and that's probably been a motivation for creating fewer but larger PRs. However, I'm wondering whether it would be feasible to split this PR into five or six smaller PRs, each with a specific purpose? For example, we could have the following as individual PRs:
Personally, I find it much easier to review PRs when they are smaller and have a single purpose; and consequently, I think I'm more likely to review them sooner. If splitting this PR up would require significant work, we can definitely keep it as is. But in case the changes are easily separable, I thought it might be worth considering a set of smaller PRs. |
|
I agree that smaller PRs are easier to review, and I am definitely guilty of making large PRs. As you might notice, when I incrementally brought MPAS functionalities from CAM to CAM-SIMA, a lot of code redesign and refactoring were involved. During the development, the code design evolved continuously as I progressed into different stages of MPAS dycore. The turnaround time was not fast enough and my code changes started to pile up. I apologize for that. However, I did make every effort to organize the commit history such that each commit serves a single purpose, and the commit messages provide the context and rationale behind each set of changes. Hopefully it helps even if this PR is large. I believe that MPAS dycore will reach a stable state after this PR. Future ones should be a lot smaller and more focused to one theme for each. |
Tag name (required for release branches):
sima0_01_001
Originator(s):
kuanchihwang
Descriptions (include the issue title, and the keyword ['closes', 'fixes', 'resolves'] followed by the issue number):
This PR implements finalization for MPAS dynamical core.
This PR also implements various enhancements for MPAS dynamical core:
usestatements to the smallest scope possible.assetsdirectory.mpas_. They are now easily distinguishable from CAM-SIMA ones. The possibility of name collisions with CAM-SIMA ones is also resolved once and for all.Describe any changes made to the build system:
Due to the slight reorganization of directory structure,
cime_config/buildlibis updated with the new paths.Describe any changes made to the namelist:
The namelist options for running analytic cases with MPAS dynamical core are updated to match CAM.
List any changes to the defaults for the input datasets (e.g., boundary datasets):
The input files for running analytic cases with MPAS dynamical core are updated to match CAM. The supported resolutions are:
mpasa480,mpasa120,mpasa60, andmpasa30with 32 vertical layers.mpasa480,mpasa120, andmpasa60with 58 vertical layers.mpasa480,mpasa120, andmpasa60with 93 vertical layers.List all files eliminated and why:
None
List all files added and what they do:
A src/dynamics/mpas/assets/0001-Prefix-all-MPAS-namelist-group-and-option-names.patchA src/dynamics/mpas/assets/0002-Disable-physics-for-MPAS-dycore-only-build.patchA src/dynamics/mpas/assets/0003-Declare-constants-at-the-native-precision-of-MPAS.patchA src/dynamics/mpas/assets/generate_namelist_definition.pyList all existing files that have been modified, and describe the changes:
M cime_config/buildlibM cime_config/namelist_definition_cam.xmlM src/dynamics/mpas/driver/dyn_mpas_subdriver.F90usestatements to the smallest scope possibleM src/dynamics/mpas/dyn_comp.F90dyn_finalusestatements to the smallest scope possibleprotectedattribute to grid/mesh variablesM src/dynamics/mpas/dyn_coupling.F90usestatements to the smallest scope possibleM src/dynamics/mpas/dyn_grid.F90usestatements to the smallest scope possibleprotectedattribute to grid/mesh variablesM src/dynamics/mpas/namelist_definition_mpas_dycore.xmlM src/dynamics/mpas/stepon.F90dyn_finalusestatements to the smallest scope possibleR src/dynamics/mpas/assets/Makefile.in.CESMsedcommands fromMakefileR src/dynamics/mpas/assets/MakefileRegression tests:
New baseline for MPAS dynamical core.
No changes to other existing tests.