-
Notifications
You must be signed in to change notification settings - Fork 35
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
Update to parametric values in prognostic convection for GFSv17 #18
Update to parametric values in prognostic convection for GFSv17 #18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some parameter tuning. Looks good.
@JongilHan66 please have a look at these three updated routines when you have a chance. |
@ChunxiZhang-NOAA I updated the deep and shallow cu schemes following Jongil's suggestion after you approved the PR, you can have a look when you get a chance. Thank you. |
@lisa-bengtsson Sure I will review it again. BTW: It would be great if there are a few slides of PPT showing how the changes of the cumulus scheme impact the results. @JongilHan66 Do you have those slides? |
@ChunxiZhang-NOAA I can update the slides I presented in the last UFS convection WG meeting with these last tests based on the tuning in this PR, do you think it is OK if that is done by Friday this week? This PR probably wont be merged before then? |
@lisa-bengtsson Sure, please attach your results you presented last time. If @JongilHan66 could also attach his testing results for his changes, it would be great. No problem, take your time. This PR is targeted to be merged around Nov 10. |
@lisa-bengtsson The changes are correct |
@ChunxiZhang-NOAA is it possible to re-run the ccpp_build step for the CI test? I think it automatically ran in between updating the ccpp/physics submodule and the FV3 submodule. Thanks |
@lisa-bengtsson The CI tests have a bug in them, that's why they are failing. I opened a tiny PR into this branch to address this. If you are okay with this we can include it in this PR? Also, @ChunxiZhang-NOAA @grantfirl Do you think it's okay we add this here? Or open a whole new PR? |
@dustinswales I can rerun the ccpp_build step for the CI tests manually. But without your bug fix being merged in, the tests will fail again. |
@ChunxiZhang-NOAA The SCM test will fail, since it's not up-to-date, but the fv3atm test will pass. |
@dustinswales I think it's OK to add the bugfix here since we're discussing it here and it's just a minor bugfix. |
It is Ok with me to add the bugfix here. |
@dustinswales I will try fv3atm test then. BTW: it is ok to add the minor bug fix here. |
Sounds good. |
Hotfix for CI tests
@dustinswales The CI tests are successful after the bug fix was merged in. |
All tests are done on ufs wm pr #1480. Please, go ahead to merge in this pr. |
modified: progsigma_calc.f90 modified: samfdeepcnv.f modified: samfshalcnv.f
These PR’s in ufs-weather-model, fv3atm and ccpp/physics does the following:
Creates a set of _gfsv17 RT’s to reflect tests that are beyond prototype 8 targeted for GFSv17
Activates prognostic closure by setting namelist progsigma = true in _gfsv17 and pointing to new field_table in the RT’s
Addresses additional parametric tuning in progclosure_calc.F90
Includes TKE contribution from cu for progsigma area fraction in samfdeep and samfshal cu schemes.
Adds a new gfsv17 field_table and diag_table
Issue: ufs-community/ufs-weather-model#1477
Dependencies: ufs-community/ufs-weather-model#1480 , NOAA-EMC/fv3atm#598
Here is a PPT describing the impact of this tuning on slide 9: https://docs.google.com/presentation/d/1m8GNn03tkwuKxuQxIt4kRcih5Ta3JDtQHy79VMBW20Y/edit#slide=id.g17f3245d7e2_0_399
Here are the summer and winter period vsdb verification:
https://www.emc.ncep.noaa.gov/gmb/jhan/vsdbw/progcw1/ (winter)
https://www.emc.ncep.noaa.gov/gmb/jhan/vsdbw/progcs1/ (summer)