Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HR4 Gravity Wave Drag Update #207

Merged
merged 14 commits into from
Jul 10, 2024

Conversation

Qingfu-Liu
Copy link
Collaborator

@Qingfu-Liu Qingfu-Liu commented May 18, 2024

The code of this PR is provided by Jongil Han.

  1. This update is a combination of the gravity wave drag (GWD) versions from the NOAA/GSL and NOAA/PSL. The PR also update the Land Noahmp code.

  2. The test results with this update can be seen in:
    a) summer: https://www.emc.ncep.noaa.gov/gmb/jhan/vsdbw/hr3e50s
    b) winter: https://www.emc.ncep.noaa.gov/gmb/jhan/vsdbw/hr3d11w
    HR4GWD: same as HR3 but with the updated GWD

    Compared to the HR2 and HR3, the HR4GWD shows a significant improvement especially in 500mb height AC and CONUS precipitation forecast skills. The HR4GWD reduces the cold and dry biases in the lower troposphere compared to the HR3. It also reduces the negative wind speed biases in the troposphere compared to the HR2.

…wp.F90 module_sf_noahmplsm.F90 noahmpdrv.F90
@Qingfu-Liu Qingfu-Liu marked this pull request as ready for review May 19, 2024 15:55
@Qingfu-Liu Qingfu-Liu marked this pull request as draft May 19, 2024 15:56
@Qingfu-Liu Qingfu-Liu marked this pull request as ready for review May 19, 2024 15:57
@Qingfu-Liu Qingfu-Liu marked this pull request as draft May 19, 2024 16:59
@Qingfu-Liu Qingfu-Liu marked this pull request as ready for review May 19, 2024 17:30
@lisa-bengtsson
Copy link
Collaborator

@JongilHan66 are there namelist switches to go with this update?

@Qingfu-Liu
Copy link
Collaborator Author

@lisa-bengtsson The namelist change related to workflow, I will contact the workflow code manager to change the repository

@lisa-bengtsson
Copy link
Collaborator

@Qingfu-Liu I understand. I am wondering what they are, so I can test on my own.

physics/SFC_Models/Land/Noahmp/noahmpdrv.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.meta Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.meta Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/module_sf_noahmplsm.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/module_sf_noahmplsm.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/module_sf_noahmplsm.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/module_sf_noahmplsm.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/module_sf_noahmplsm.F90 Outdated Show resolved Hide resolved
physics/SFC_Models/Land/Noahmp/noahmpdrv.F90 Outdated Show resolved Hide resolved
@Qingfu-Liu
Copy link
Collaborator Author

@lisa-bengtsson Can you take a look of the compile problem for suite suite_FV3_HRRR_c3.xml ? There is a conflict with the new code.
See Dusan's test error:
https://github.com/NOAA-EMC/fv3atm/actions/runs/9142433918/job/25137982061?pr=836#step:6:2123
Thank you very much

@barlage
Copy link
Collaborator

barlage commented May 22, 2024

@Qingfu-Liu can you merge the latest develop? there are noahmp changes that are not included in your branch.

@Qingfu-Liu
Copy link
Collaborator Author

@Qingfu-Liu can you merge the latest develop? there are noahmp changes that are not included in your branch.

@barlage OK. I will merge the new change.

@Qingfu-Liu
Copy link
Collaborator Author

New code is uploaded, but there is problem to compile the new code. The CCPP-physics repository has updated significantly, there is a conflict to merge the new code. Currently working on it.

@mdtoyNOAA
Copy link
Collaborator

Our sprawling GWD parameterizations need some love. When doing the repository refactoring, it was the only process that couldn't be organized like the other processes, and it still has a flat directory structure reflecting this lack of "organizability"
There's a mixture of nml and SDF control, calling CCPP entrypoints from wrappers using nml options. Sometimes dra_suite_run is exposed at the SDF levels, other times it is referenced from a wrapper. Other GWD schemes are intertwined in a similar manner
I'm not going to hold these changes up for HR4, but if we prioritized cleanup efforts, this would be on top of the list.

I agree wholeheartedly. We have brought this up many times in the past, although we did not consider it imperative because it was still in research and develop mode. If this is marching toward operations, I would be strongly in favor of cleaning up the code organization too.

Code clean-up is on my to-do list. I think when the dust settles and we have a final version for GFSv17 would be the time to do it.

@yangfanglin
Copy link
Collaborator

Code clean-up is on my to-do list. I think when the dust settles and we have a final version for GFSv17 would be the time to do it.

Mike, as we discussed before we need to reorganize and remove some of the legacy code as well. There are too many versions and some duplicated namelist options.

@grantfirl
Copy link
Collaborator

@Qingfu-Liu @mdtoyNOAA The plan is to combine #210 into this one? Does anyone need any help debugging/testing to move this along?

@JongilHan66
Copy link
Collaborator

@Qingfu-Liu @mdtoyNOAA The plan is to combine #210 into this one? Does anyone need any help debugging/testing to move this along?

@grantfirl The "alpha_fd" in #210 is already combined into the updated codes, so we don't need PR#210 any more.

@grantfirl
Copy link
Collaborator

@Qingfu-Liu @mdtoyNOAA The plan is to combine #210 into this one? Does anyone need any help debugging/testing to move this along?

@grantfirl The "alpha_fd" in #210 is already combined into the updated codes, so we don't need PR#210 any more.

Thanks @JongilHan66 . I'll close #210, I just didn't see the "updated codes" reflected in this PR yet. I'm assuming that @Qingfu-Liu is working on it locally and hasn't pushed to GitHub yet (which is fine).

@Qingfu-Liu
Copy link
Collaborator Author

@Qingfu-Liu @mdtoyNOAA The plan is to combine #210 into this one? Does anyone need any help debugging/testing to move this along?

@grantfirl The "alpha_fd" in #210 is already combined into the updated codes, so we don't need PR#210 any more.

Thanks @JongilHan66 . I'll close #210, I just didn't see the "updated codes" reflected in this PR yet. I'm assuming that @Qingfu-Liu is working on it locally and hasn't pushed to GitHub yet (which is fine).

I just able to compile with the new update from the GWD. I will commit the new changes soon

@Qingfu-Liu
Copy link
Collaborator Author

All the code for HR4 GWD and Noahmp are just updated based on the new suggestions

@Qingfu-Liu
Copy link
Collaborator Author

@Qingfu-Liu @mdtoyNOAA The plan is to combine #210 into this one? Does anyone need any help debugging/testing to move this along?

Hi @grantfirl I have uploaded all the code and only run tests for two suites. If you have time, could you please run all the suites tests. Thank you very much

@grantfirl
Copy link
Collaborator

@Qingfu-Liu @mdtoyNOAA The plan is to combine #210 into this one? Does anyone need any help debugging/testing to move this along?

Hi @grantfirl I have uploaded all the code and only run tests for two suites. If you have time, could you please run all the suites tests. Thank you very much

OK, I'll run them now.

@grantfirl
Copy link
Collaborator

@Qingfu-Liu I've kicked off the full rt.conf on Hera. I'll help debug if there are any unexpected failures.

@grantfirl
Copy link
Collaborator

@Qingfu-Liu Please merge in Qingfu-Liu#3 for some minor fixes.

@Qingfu-Liu
Copy link
Collaborator Author

@Qingfu-Liu Please merge in Qingfu-Liu#3 for some minor fixes.

@grantfirl I just merged Qingfu-Liu#3 to PR#207

@grantfirl
Copy link
Collaborator

@Qingfu-Liu Please merge in Qingfu-Liu#3 for some minor fixes.

@grantfirl I just merged Qingfu-Liu#3 to PR#207

@Qingfu-Liu I verified the merge. Looks good!

@grantfirl grantfirl requested a review from barlage June 3, 2024 17:04
@Qingfu-Liu
Copy link
Collaborator Author

@Qingfu-Liu Please merge in Qingfu-Liu#3 for some minor fixes.

@grantfirl I just merged Qingfu-Liu#3 to PR#207

@Qingfu-Liu I verified the merge. Looks good!

@grantfirl Thanks

@grantfirl grantfirl dismissed barlage’s stale review June 14, 2024 14:38

Requested changes have been addressed.

@Qingfu-Liu
Copy link
Collaborator Author

The changes of the NoahMP model (which are not used for HR4-GWD-update tests) have been retracted. All the regression tests are passed on Hera

do k = kpblmax,km
if ((taud_ls(i,k)+taud_bl(i,k)).ne.0..and.prsl(i,k).le.pcutoff) then
denfac = min(ro(i,k)/pcutoff_den,1.)
dtfac(i,k) = min(dtfac(i,k),denfac*abs(velco(i,k) &
Copy link
Collaborator

@grantfirl grantfirl Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JongilHan66 This is the line causing the latest trouble in ufs-community/ufs-weather-model#2290. The problem is that this look is going up to 'km' when velco is only defined up to km-1. Would it work to simply change the loop from kpblmax to km-1, or does this mess up something else?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know, and I can make the change and test that it works real quick.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know, and I can make the change and test that it works real quick.

@grantfirl I don't think the change causes a problem. Please change the loop from kpblmax to km-1.
BTW, in 'drag_suite.meta', 'vtype' and 'psl_gwd_dx_factor' were removed from the original meta file. Is it ok?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JongilHan66 Yes, they should have been removed because drag_suite_psl() is not actually exposed to the CCPP. It is only being called from within ugwpv1_gsldrag_run or unified_ugwp_run. Both the ugwpv1_gsldrag and unified_ugwp schemes have those two variables accessible to pass in to drag_suite_psl already.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JongilHan66 Yes, they should have been removed because drag_suite_psl() is not actually exposed to the CCPP. It is only being called from within ugwpv1_gsldrag_run or unified_ugwp_run. Both the ugwpv1_gsldrag and unified_ugwp schemes have those two variables accessible to pass in to drag_suite_psl already.

@grantfirl I see. Thanks!!

…broutine; change upper bound of loop to prevent array bound excursion
@grantfirl grantfirl merged commit 8103e21 into ufs-community:ufs/dev Jul 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants