ufs-community / ufs-weather-model

UFS Weather Model
Other
130 stars 238 forks source link

add mynn surface scheme as opt_sfc=4 in NoahMP #1224

Closed HelinWei-NOAA closed 1 year ago

HelinWei-NOAA commented 2 years ago

PR Checklist

issue 920 in ccpp-physics

Instructions: All subsequent sections of text should be filled in as appropriate.

The information provided below allows the code managers to understand the changes relevant to this PR, whether those changes are in the ufs-weather-model repository or in a subcomponent repository. Ufs-weather-model code managers will use the information provided to add any applicable labels, assign reviewers and place it in the Commit Queue. Once the PR is in the Commit Queue, it is the PR owner's responsiblity to keep the PR up-to-date with the develop branch of ufs-weather-model.

Description

This PR is to add MYNN surface scheme as opt_sfc=4 in NoahMP and recode the thermal roughness scheme as a subroutine. A bug with opt_sfc=1 was fixed. 2m T/Q from NoahMP instead of sfc_diag will be used as outputs.

The update is not aimed for UFS coupled prototype 8, but for RRFS.

Issue(s) addressed

Link the issues to be closed with this PR, whether in this repository, or in another repository. (Remember, issues must always be created before starting work on a PR branch!)

Testing

How were these changes tested? What compilers / HPCs was it tested with? Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) Have regression tests and unit tests (utests) been run? On which platforms and with which compilers? (Note that unit tests can only be run on tier-1 platforms)

Dependencies

If testing this branch requires non-default branches in other repositories, list them. Those branches should have matching names (ideally).

Do PRs in upstream repositories need to be merged first? If so add the "waiting for other repos" label and list the upstream PRs

jkbk2004 commented 2 years ago

@HelinWei-NOAA I think we can start working on this PR. Can you sync up your branches to develop/main: ccpp and fv3atm?

github-actions[bot] commented 2 years ago

@HelinWei-NOAA please bring these up to date with respective authoritative repositories

HelinWei-NOAA commented 2 years ago

Which submodules are not up to date? Thanks.

@HelinWei-NOAA please bring these up to date with respective authoritative repositories

  • fv3 NOT up to date
DeniseWorthen commented 2 years ago

@SamuelTrahanNOAA Please check the changes in tests/parm/rrfs_conus13km_hrrr.nml.IN

DusanJovic-NOAA commented 2 years ago

Something is wrong with this PR. Description says it depends on fv3atm PR 537, but .gitmodules does not point to a fork with that PR, it still points to NOAA-EMC

SamuelTrahanNOAA commented 2 years ago

Could @joeolson42 please review these changes?

jkbk2004 commented 2 years ago

Could @joeolson42 please review these changes?

@SamuelTrahanNOAA added @joeolson42 to the repo.

HelinWei-NOAA commented 2 years ago

At this moment I have reverted all opt_sfc changes. In the future we will redesign some RTs using opt_sfc=4.

SamuelTrahanNOAA commented 2 years ago

At this moment I have reverted all opt_sfc changes. In the future we will redesign some RTs using opt_sfc=4.

You have not reverted the changes to the conus13km namelist.

jkbk2004 commented 2 years ago

@HelinWei-NOAA can you address the reviewers' comments above?

HelinWei-NOAA commented 2 years ago

It is weird that I didn't make any of those changes Sam mentioned except for opt_sfc in my original commit

SamuelTrahanNOAA commented 1 year ago

It is weird that I didn't make any of those changes Sam mentioned except for opt_sfc in my original commit

You may have gotten it from an earlier version of the develop branch, before the big GSL merge a few weeks ago.

jkbk2004 commented 1 year ago

@HelinWei-NOAA I tested to build on hera. An error occured while running ccpp_prebuild.py Can you check the compile failure? /scratch1/NCEPDEV/stmp2/Jong.Kim/FV3_RT/rt_19438/compile_001/err

HelinWei-NOAA commented 1 year ago

It looks like the wrong version of rte-rrtmgp is used in my ccpp-physics repository. I am making it right now.

jkbk2004 commented 1 year ago

@HelinWei-NOAA One thing to check. Since changes in RTs are reverted, do you think this PR will still change baseline?

HelinWei-NOAA commented 1 year ago

@HelinWei-NOAA One thing to check. Since changes in RTs are reverted, do you think this PR will still change baseline?

Yes for those tests with Noah MP. We have some refinements and bug fix for Noah MP.

jkbk2004 commented 1 year ago

@HelinWei-NOAA OK! I will submit auto-RT runs around noon time.

jkbk2004 commented 1 year ago

@HelinWei-NOAA can you test on your side? I still see some build issue on ccpp side. /glade/scratch/jongkim/UFS-RT-tests/rt-1224/FV3/ccpp/physics/physics/module_sf_noahmplsm.f90:4582:32:

4582 | z0mo,z0h) | 1 Error: Type mismatch in argument ‘icom’ at (1); passed INTEGER(4) to INTEGER(8)

HelinWei-NOAA commented 1 year ago

Fixed the issue. Compiling went through on my own test.

@HelinWei-NOAA can you test on your side? I still see some build issue on ccpp side. /glade/scratch/jongkim/UFS-RT-tests/rt-1224/FV3/ccpp/physics/physics/module_sf_noahmplsm.f90:4582:32:

4582 | z0mo,z0h) | 1 Error: Type mismatch in argument ‘icom’ at (1); passed INTEGER(4) to INTEGER(8)

junwang-noaa commented 1 year ago

@HelinWei-NOAA Just to check, do you plan to add a test with opt_sfc=4 in NoahMP in the future? Otherwise this feature may not be maintained as there is no test.

HelinWei-NOAA commented 1 year ago

@HelinWei-NOAA Just to check, do you plan to add a test with opt_sfc=4 in NoahMP in the future? Otherwise this feature may not be maintained as there is no test.

They plan to use opt_sfc=4 for RRFS. What feature may not be maintained? @barlage Should we change one of RRFS RTs with opt_sfc=4?

SamuelTrahanNOAA commented 1 year ago

Can you copy the tests/tests/rap_control_debug to a new tests/tests file, and change iopt_sfc? Then add it to rt.sh and rt_gnu.sh next to rap_control_debug and you'll have a new test.

HelinWei-NOAA commented 1 year ago

@SamuelTrahanNOAA RUC not Noah MP lsm is used in the test rap_control_debug. Noah MP with opt_sfc=4 is planned for all future RRFS runs. That's why I changed those at the beginning.

SamuelTrahanNOAA commented 1 year ago

What about rap_noah_debug?

barlage commented 1 year ago

@HelinWei-NOAA @SamuelTrahanNOAA it looks like the four rrfs_v1* tests use noahmp and MYNN; should all four be set to use this new iopt_sfc=4?

SamuelTrahanNOAA commented 1 year ago

@joeolson42 Could you please respond to @barlage's question?

joeolson42 commented 1 year ago

No, unfortunately I can not answer this question. I'm not sure which LSM is meant to be used. Perhaps the name of the SDF should reflect it.

jkbk2004 commented 1 year ago

@HelinWei-NOAA @SamuelTrahanNOAA @junwang-noaa What about creating issue to add additional tests or extensively turn on iopt_sfc=4? so that we can move on merging the PR now.

SamuelTrahanNOAA commented 1 year ago

As long as the conus13km tests aren't changed, I have no objection. Were it my PR though, I would want a regression test before it is merged. It is not, however, my PR nor my decision.

jkbk2004 commented 1 year ago

If no objection, I will submit auto-RT runs to create new baseline this evening.

HelinWei-NOAA commented 1 year ago

The four rrfs_v1* tests use noahmp and MYNN. For consistence it is better to use MYNN scheme in Noah MP (opt_sfc=4) too. It is not about to decide which LSM is to be used. Noah MP is already used in these four RTS. @SamuelTrahanNOAA If you agree, I am going to change opt_sfc=4 for those 4 RTs. @barlage

No, unfortunately I can not answer this question. I'm not sure which LSM is meant to be used. Perhaps the name of the SDF should reflect it.

jkbk2004 commented 1 year ago

@HelinWei-NOAA Sounds like maintaining some tests with the opt_sfc=4 is acceptable according to the conversation, can you move on to update? so that we can start creating new baseline.

HelinWei-NOAA commented 1 year ago

@HelinWei-NOAA Sounds like maintaining some tests with the opt_sfc=4 is acceptable according to the conversation, can you move on to update? so that we can start creating new baseline.

Done. Changed opt_sfc=4 in rap.nml.IN. Only affect those 4 rrfs_v1* tests. Noah MP is not used in the other tests using rap.nml.IN. So there is no impact on them.

BrianCurtis-NOAA commented 1 year ago

Automated RT Failure Notification Machine: orion Compiler: intel Job: BL [BL] Repo location: /work/noaa/nems/emc.nemspara/autort/pr/941988484/20220616091510/ufs-weather-model [BL] Error: Test rrfs_v1beta 031 failed in run_test failed [BL] Error: Test rrfs_v1nssl 032 failed in run_test failed [BL] Error: Test rrfs_v1nssl_nohailnoccn 033 failed in run_test failed [BL] Error: Test rrfs_v1beta_debug 069 failed in run_test failed Please make changes and add the following label back: orion-intel-BL

BrianCurtis-NOAA commented 1 year ago

/work/noaa/nems/emc.nemspara/autort/pr/941988484/20220616091510/ufs-weather-model

137: [Orion-16-22:370810:0:370810] Caught signal 11 (Segmentation fault: address not mapped to object at address 0xfff ffffc2133a128) 46: An error occurred in ccpp_physics_run for group physics, block 1 and thread 1 (ntX= 1): 46: An error occured in noahmpdrv_run: erreng = 157.449802856605 at i,j: 3 -9999 46: net solar: 163.9410 46: net longwave: 65.9686 46: total sensible: ** 46: canopy evap: 0.0069 46: ground evap: 105.8756 46: transpiration: 0.8587 46: total ground: ** 46: canopy heat stora 6.4913 46: precip advected: 0.0001 -0.0002 0.0000 0.0004 46: precip: 0.0000 46: veg fraction: 0.1926 46: energy budget problem in noahmp lsm

HelinWei-NOAA commented 1 year ago

/work/noaa/nems/emc.nemspara/autort/pr/941988484/20220616091510/ufs-weather-model

@BrianCurtis-NOAA Can you open the permission? All rrfs_v1* tests failed?

BrianCurtis-NOAA commented 1 year ago

/work/noaa/nems/emc.nemspara/autort/pr/941988484/20220616091510/ufs-weather-model

@BrianCurtis-NOAA Can you open the permission? All rrfs_v1* tests failed?

can you access: /work/noaa/stmp/bcurtis/stmp/bcurtis/FV3_RT/rt_397081 ? that is where the runs are.

HelinWei-NOAA commented 1 year ago

/work/noaa/nems/emc.nemspara/autort/pr/941988484/20220616091510/ufs-weather-model

@BrianCurtis-NOAA Can you open the permission? All rrfs_v1* tests failed?

can you access: /work/noaa/stmp/bcurtis/stmp/bcurtis/FV3_RT/rt_397081 ? that is where the runs are.

Yes. Thanks.

jkbk2004 commented 1 year ago

@HelinWei-NOAA when fixes are made, we can restart working on the PR.

junwang-noaa commented 1 year ago

@HelinWei-NOAA With the changes you made in rap.nml.IN, it looks to me all the rap tests will be impacted. Can you check?

HelinWei-NOAA commented 1 year ago

I am working on the issue.

HelinWei-NOAA commented 1 year ago

@HelinWei-NOAA With the changes you made in rap.nml.IN, it looks to me all the rap tests will be impacted. Can you check?

@junwang-noaa No. None of rap tests use Noah MP. opt_sfc is a valid flag only when Noah MP is used.

HelinWei-NOAA commented 1 year ago

@HelinWei-NOAA when fixes are made, we can restart working on the PR.

The issue has been fixed. My own 4 rrfs_v1* RTs went through. The cause is a code mistake made when I tried to address reviewer comments.

HelinWei-NOAA commented 1 year ago

@junwang-noaa If you want me to bring this issue into this PR, I can make the change too.

jkbk2004 commented 1 year ago

@HelinWei-NOAA we are in the middle of merging #1294. @ChunxiZhang-NOAA can combine issue #1274 in his PR. So we might be able to work on this PR from tomorrow, I think. Will keep you posted.

ChunxiZhang-NOAA commented 1 year ago

@HelinWei-NOAA @jkbk2004 Great! Thanks!

jkbk2004 commented 1 year ago

@HelinWei-NOAA we will start working on creating new BL. issue 1274 will be combined with #1192.

jkbk2004 commented 1 year ago

@HelinWei-NOAA can you confirm it builds ok? I see compilation failure: CMake Error at FV3/ccpp/CMakeLists.txt:44 (message): An error occured while running ccpp_prebuild.py, check

jkbk2004 commented 1 year ago

@HelinWei-NOAA ccpp in your branch might be behind.

HelinWei-NOAA commented 1 year ago

I am checking and testing now.