Bug fix for the NoahMP scheme #1493
Conversation
|
@ChunxiZhang-NOAA This change might affect the external land component too. At this point, we have two copy of same source files (under NoahMP external component vs. CCPP/Physics). It seems that the PR is adding additional variable to the noahmpdrv (ztmax) and needs to be coordinated with the land group. @barlage I think we need also port these mods to NOAA-EMC/noahmp repository. Otherwise, there will be two different version of same code and probably external land component related configurations will fail. @junwang-noaa @barlage I have a version of code that defines noahmp related files as external under CCPP/Physics (see https://github.com/uturuncoglu/ccpp-physics/tree/feature/lnd_noahmp_new_submodule). Maybe we need to go with that way and that ensures using same code base for noahmp. Of course, we still need to update the code in both place but at least both code will point same repo. |
|
@ChunxiZhang-NOAA I agree with Ufuk. It's good that we use the same version of NoahMP under CCPP physics and in the land component. @uturuncoglu @barlage Is the https://github.com/esmf-org/noahmp the official noahmp repository? |
|
@junwang-noaa the official repository is in here: https://github.com/NOAA-EMC/noahmp |
|
@uturuncoglu I saw your branch https://github.com/uturuncoglu/ccpp-physics/tree/feature/lnd_noahmp_new_submodule is pointing to https://github.com/esmf-org/noahmp. Can you merge the code updates from esmf noahmp to the official noahmp branch and use that under CCPP? |
|
@junwang-noaa Yes. That was old work before we have external noahmp land PR. So, it points esmf-org. Anyway, there is no any additional development in esmf-org in-terms of noahmp. So, if you want to go with external module approach under CCPP/Physics? We just need to have CCPP/Physics fork that has modifications for external. Do you want me create that in a branch of my fork? Then, maybe @ChunxiZhang-NOAA will work with that to bring his changes to NOAA-EMC/noahmp repository. BTW, this changes also requires minor modification in the noahmp NUOPC cap since we are also driving noahmpdrv code from cap. |
@uturuncoglu I can make the changes to NOAA-EMC/noahmp repository. |
|
@ChunxiZhang-NOAA Okay. You could see all the changes that I did in CCPP/Physics side in this commit - uturuncoglu/ccpp-physics@887b8c2 Then, I think you will move your noahmp related modifications to your NOAA-EMC/noahmp fork and open a PR in there. The PR will be tested automatically through the GitHub Action that I defined in NOAA-EMC/noahmp (this is the first time we will try it). Please also note that noahmp driver is also called in here, https://github.com/NOAA-EMC/noahmp/blob/0729e8857609ef5b6c214d270ce4baf372c59b08/drivers/nuopc/lnd_comp_driver.F90#L554 and you need to introduce your new variable also in there. BTW, the noahmp code which is found in the NOAA-EMC/noahmp should be up-to-date with the one found in CCPP/Physics but it has some fixed for GNU compiler. You could see them in the following history, https://github.com/NOAA-EMC/noahmp/commits/develop/src/module_sf_noahmplsm.f90 |
|
@ChunxiZhang-NOAA BTW, Let me know if you need help. |
|
@ChunxiZhang-NOAA I think it would be also nice to go step by step and running couple of RT in each one to ensure that we are not breaking anything by adding NOAHMP as external under CCPP/Physics. |
|
@ChunxiZhang-NOAA please bring these up to date with respective authoritative repositories
|
|
@uturuncoglu I think we should postpone this update of noahmp in the component model until we can get the NOAA-EMC fork working in both ccpp and the component. It was my understanding that we would let these codes diverge a bit at this point since the goal of the #1443 was to set up the infrastructure for the component land and not necessarily execute since the two-way coupling is still not functional. We should plan on getting the noahmp submodule in ccpp-physics for global prototype HR2. The proposed bug fixes are crucial for HR1. |
|
@barlage There is no option to have two different version of noahmp under same modeling system (external component vs. CCPP/Physics). Of course you could change the module names (either in CCPP or external component) to prevent namespace collusion if you want to go in that direction but I think that will make more complex to maintain the code. |
|
@uturuncoglu Maybe I'm not clear here. Currently, we have a version of noahmp that resides in ccpp-physics and a version that you are manually updating to esmf-org/noahmp that is used in the component model. Ultimately, we want NOAA-EMC/noahmp to be used in both ccpp-physics and the component model. (as a side note, we also eventually want NOAA-EMC/noahmp to be a true fork (it currently is not) of NCAR/noahmp, which is the community noahmp repo). My main point is that this PR is addressing a few bugs that are crucial for the ccpp-version, but not so crucial for how the component model is currently being used. As a follow-on to this PR, we should sync both ccpp-physics and the component model to NOAA-EMC/noahmp. |
|
@uturuncoglu OK, maybe I'm wrong on this. I thought you had a noahmp submodule to esmf-org, but maybe that is not the case: https://github.com/ufs-community/ufs-weather-model/tree/develop/NOAHMP-interface |
|
@barlage since we have two copy of same code and their module names are same, having different version of code could lead unexpected issues. So, in the run time the code chose one of them randomly and that could be the one under CCPP/physics. In particular to this PR, it introduces additional argument to noahmpdrv which is not available in the component version and this could lead to crash in the configurations with external land component. There are two solution in here,
I prefer to go with option (1) in here since prevents to have two different version of the code in the same modeling system. Also, ensures side-by-side RT test will continue to give similar answer with CCPP one. Otherwise, there is no meaning to have side-by-side RT because once the code diverges you won't get similar results anymore. Anyway, I think if this PR fix something in the noahmp, it must fix in both side. |
|
@barlage as I mentioned before to @junwang-noaa, the branch that introduces noahmp as a sub-component is using esmf.org since it is created before our initial PR. There is no restriction to apply the same method to add noaa-emc/noahmp repo to ccpp/physics. So, then both external land and ccpp/physics could point noaa-emc/noahmp develop branch. |
|
@barlage @junwang-noaa i am happy to discuss this in the todays call, if you want. |
|
@uturuncoglu do we have a call today? I agree that (1) is the way forward and what I was trying to propose for a follow-on PR. This will require quite a lot of work and testing, IMO. Isn't it also possible that as a stopgap we can just add the additional variable to the component model interface? |
|
@barlage Sorry, I mean "ufs weather model infrastructure development meeting" call at 11 MT. |
|
@barlage There could be also possibility to use special and new CPP pre-processor to bring those modifications but I think it wold make the code more complex and again side-by-side RT will not have meaning anymore. We need to default configuration of external land component as mush as possible to the CCPP/Physics one since community might need to have some experiments and comparison between them until they convinced that external land component produces reasonable results when it is compared with CCPP/Physics. |
|
I think CPP way will not work since at the end we will different number of arguments for same subroutine. Sorry about confusion. |
|
@uturuncoglu @barlage @junwang-noaa Ok, I think we have made an agreement for what we will add in this PR. I will create a fork from NOAA-EMC/noahmp, then add the modified files and other changes to the external land component. I will create a PR for the external land component. |
|
@uturuncoglu Currently the ufs-weather-model points to the noahmp hash that is a few commits behind the develop branch, shall I take this chance to make it point to the latest hash after my commit merged in? |
on-behalf-of @ufs-community <[email protected]>
on-behalf-of @ufs-community <[email protected]>
on-behalf-of @ufs-community <[email protected]>
on-behalf-of @ufs-community <[email protected]>
|
@ChunxiZhang-NOAA we can merge in here after updates on noahmp and fv3 pointers and reverting gitmodules branches. |
|
@jkbk2004 Done. |
PR Checklist
This PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR. Please consult the ufs-weather-model wiki if you are unsure how to do this.
This PR has been tested using a branch which is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR
An Issue describing the work contained in this PR has been created either in the subcomponent(s) or in the ufs-weather-model. The Issue should be created in the repository that is most relevant to the changes in contained in the PR. The Issue and the dependent sub-component PR
are specified below.
Results for one or more of the regression tests change and the reasons for the changes are understood and explained below.
New or updated input data is required by this PR. If checked, please work with the code managers to update input data sets on all platforms.
Description
This PR is created on behalf of @HelinWei-NOAA and EMC land team.
This PR aims to fix the ccpp-physics bugs that are reported in NCAR/ccpp-physics#963 and NCAR/ccpp-physics#964. It will be included in HR1. This PR will change the baselines for the RTs that use the NoahMP scheme.
Issue(s) addressed
Testing
All RTs using NoahMP failed due to the results change. Besides those failed tests, merra2_thompson failed to run due to the instability issue. The run was successful if changing dt_atmos from 720s to 600s. But still many Warn_K warnings. An Issue#24 related to this instability problem has been created. A full ORT test that based on control_p8 passed. The failed RTs are:
cpld_control_p8 001 failed in check_result
cpld_2threads_p8 003 failed in check_result
cpld_esmfthreads_p8 004 failed in check_result
cpld_decomp_p8 005 failed in check_result
cpld_mpi_p8 006 failed in check_result
cpld_control_ciceC_p8 007 failed in check_result
cpld_control_c192_p8 008 failed in check_result
cpld_bmark_p8 010 failed in check_result
cpld_control_noaero_p8 012 failed in check_result
cpld_control_nowave_noaero_p8 013 failed in check_result
cpld_debug_p8 014 failed in check_result
cpld_debug_noaero_p8 015 failed in check_result
cpld_control_noaero_p8_agrid 016 failed in check_result
cpld_control_c48 017 failed in check_result
cpld_warmstart_c48 018 failed in check_result
control_p8 040 failed in check_result
control_p8_lndp 041 failed in check_result
control_decomp_p8 043 failed in check_result
control_2threads_p8 044 failed in check_result
control_p8_rrtmgp 045 failed in check_result
rrfs_v1beta 068 failed in check_result
rrfs_v1nssl 069 failed in check_result
rrfs_v1nssl_nohailnoccn 070 failed in check_result
control_debug_p8 092 failed in check_result
rrfs_v1beta_debug 107 failed in check_result
datm_cdeps_lnd_gswp3 152 failed in check_result
control_p8_atmlnd_sbs 154 failed in check_result
atmaero_control_p8 156 failed in check_result
atmaero_control_p8_rad 157 failed in check_result
atmaero_control_p8_rad_micro 158 failed in check_result
merra2_thompson 046 failed in run_test
Dependencies