NCAR / ccpp-physics

Collection of physics parameterizations compliant with the Common Community Physics Package (CCPP) framework
Other
60 stars 146 forks source link

Physics changes for new ccpp framework (Capgen) #1085

Open dustinswales opened 2 months ago

dustinswales commented 2 months ago

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:

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.

grantfirl commented 2 months ago

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 https://github.com/ufs-community/ccpp-physics/pull/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.

dustinswales commented 2 months ago

@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.

@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?

dustinswales commented 2 months ago

@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...) @grantfirl I agree with focusing on the PRs you mentioned before touching this (thick) PR.

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?

barlage commented 2 months ago

@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.

@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?

@dustinswales no issue for me, I already approved and was just curious if there were any plans for this as I prepare other modifications.

climbfuji commented 1 month ago

Thanks for making the MPI_Comm changes. I am still not sure how we want to test this. Did you decide if you want to take this to the UFS first for testing, or keep it here for testing with SCM, and then test in the UFS? Will the changes here work with ccpp_prebuild.py or do they require updating the framework at the same time (much more difficult to test)?

dustinswales commented 1 month ago

@climbfuji I've tested these changes with the SCM using Prebuild and they are B4B. I still need to test with the UFS. @grantfirl If I'm going to test in with the UFS, I might as well close this PR down and move this into the ufs/dev fork? Then this can follow our typical UFS-fork-> Root route. Thoughts?