Physics changes for new ccpp framework (Capgen) #1085
Conversation
…ges_for_capgen
dustinswales
requested review from
grantfirl,
Qingfu-Liu,
pjpegion,
lisa-bengtsson,
haiqinli,
wd20xw,
tanyasmirnova,
barlage,
cenlinhe,
HelinWei-NOAA,
JongilHan66,
joeolson42,
mjiacono,
WeiguoWang-NOAA,
RuiyuSun,
gthompsnWRF and
AndersJensen-NOAA
as code owners
dustinswales
requested review from
AnningCheng-NOAA,
andrewgettelman and
mzhangw
as code owners
| @@ -58,6 +58,17 @@ foreach(typedef_module ${TYPEDEFS}) | |||
| endforeach() | |||
|
|
|||
| #------------------------------------------------------------------------------ | |||
| # Set the sources: kinds file | |||
| # DJS2024: This file is autogenerated by the framework (Capgen) | |||
There was a problem hiding this comment.
FYI. I haven't modified the physics to use this autogenerated type yet. The physics schemes still reference physics/hooks/machine.F to access the working precision. In the "post-capgen cleanup phase", I plan to implement the framework autogenerated kinds within the physics
dustinswales
changed the title
| @@ -54,7 +54,8 @@ module sfc_diff | |||
| !!\f] | |||
| !! - Calculate the exchange coefficients:\f$cm\f$, \f$ch\f$, and \f$stress\f$ as inputs of other \a sfc schemes. | |||
| !! | |||
| subroutine sfc_diff_run (im,rvrdm1,eps,epsm1,grav, & !intent(in) | |||
| subroutine sfc_diff_run ( & | |||
There was a problem hiding this comment.
@gold2718 Here is a Fortran parsing issue I couldn't understand.
In its original form, I was getting an odd error:
-- Invalid dummy argument, 'ivegsrc', at /scratch1/BMC/gmtb/Dustin.Swales/framework/capgen/capgen_in_scm/ccpp-scm/ccpp/physics/physics/SFC_Layer/UFS/sfc_diff.f:88
The problem seems to be the !intent(in) comment. If I remove it, the parsing works. (I just moved the args down a line so that "im" and "ps" line up.)
There was a problem hiding this comment.
@dustinswales I approve changes to lsm_ruc.F90 and lsm_ruc.meta
There was a problem hiding this comment.
@dustinswales I see you use noahmp as an example in the PR description, but currently noahmp does not have a lsm_noahmp_run subroutine. Are you planning to add that? I'm in progress with submoduling noahmp in CCPP. During that process I could change the naming to be as in your examples, which would make it more consistent with the other LSMs. Was probably going to do something like that anyway.
There was a problem hiding this comment.
A monumental effort!
There is no way to reliably review this. And because the PR started from NCAR main, I don't know how easy it will be to test this with the UFS.
All mpicomm variables must use type MPI_Comm and not integer. If there is a discrepancy between the scheme and the metadata, then the scheme must be fixed, not the metadata.
Agreed on both points. I wonder if some of these changes have already made their way into either the ufs/dev branch or even the RRFS.v1 branch and would get to main without this PR? For example, the MPI communicator stuff was covered in ufs-community#160. I'm wondering what the best way to review and test this is? Is there a timeline that this needs to be merged? If not, I'm wondering if it would be easier to do the RRFS.v1 -> ufs/dev merge (that @dustinswales has been emailed about and is aware of) and the ufs/dev -> NCAR/main merge (that we need to catch up on after the release) before even attempting to thoroughly review this. I'm worried about a bunch of merge conflicts. |
@barlage Sorry, I was writing faster than I could think and made an error referencing the noahmp lsm in the PR description. I updated the description to use noah lsm. I changed lsm_noahmp to ilsm_noahmp just to be consistent with the other lsm's, but I could revert the noahmp scheme flag change if you prefer? |
|
@climbfuji Thanks for catching those errors. I need to do some more testing on this using the SCM, I'll get it all square tomorrow (You should've seen this branch yesterday... I started this 10 months ago and my code base was a disaster, and there were the changes to optional arguments, mpi requirement, etc...) Even though this PR is for Capgen in the SCM, this PR could take the ufs/dev route for testing considerations. Given the extent of these changes, manual review will be tough, so maybe we lean on the testing the UFS has? |
@dustinswales no issue for me, I already approved and was just curious if there were any plans for this as I prepare other modifications. |
This PR contains changes needed for the next-generation ccpp-framewok, Capgen.
Most of the changes here relate to reordering the metadata to match the source argument list (Ordering in metadata files matters with Capgen!) However, there are other changes included here:
Correct Fortran/Metadata mismatches. Capgen parses the source files and metadata files, creates header tables for both, and compares them. Prebuild only parsed the metadata files, so with Capgen we are catching quite a few inconsistencies (e.g. mismatches in argument intent and type)
Adding CCPP metadata headers for all CCPP entry points. Some schemes had initialization/finalize phases in their meta files, but are missing CCPP argument tables in their source files. This wasn't causing any problems since these init/final schemes weren't doing anything, but this was a bug.
LSM control flags. In Prebuild, the CCPP API provides the Group Caps a DDT with all the interstitial data, which is referenced in the call lists to the Schemes. In Capgen, the CCPP API provides flat fields to the Group Caps, which are referenced directly in the Scheme call lists. For example, Group calls to the noah lsm:
use lsm_noah, only: lsm_noah_run
In Prebuild Caps:
call lsm_noah_run(..., lsm_noah=physics%Model%lsm_noah, ...)
In Capgen Caps:
call lsm_noah_run(..., lsm_noah=lsm_noah, ...)
In Capgen, we have a conflict between a local variable name and the module name, lsm_noah, whereas this wasn't an issue with Prebuild since the local variable was referenced in the caps as part of its parent DDT.
While this is a new tiny limitation of Capgen wrt Prebuild, it is consistent with the fortran standard.
@climbfuji @grantfirl @mkavulich I am still working through some issues with Capgen in the SCM, but these physics changes have been stable for sometime now, and they "shouldn't" change the results. I haven't tested these changes in the UFS fork.