-
Notifications
You must be signed in to change notification settings - Fork 249
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
[production/RRFS.v1] Thompson-Eidhammer microphysics code formatting #2161
[production/RRFS.v1] Thompson-Eidhammer microphysics code formatting #2161
Conversation
@MatthewPyle-NOAA @AndersJensen-NOAA This PR is ready to test. All tests that use Thompson MP are expected to change baseline results. Since this is for a release, I'm assuming that we'll want to run verification statistics on this code as well to make sure the skill doesn't change right before release? |
@grantfirl the branch looks synced ok. hera is down today. I will go ahead to run tests on orion/hercules. @MatthewPyle-NOAA FYI |
@jkbk2004 What is the best practice for a PR such as this one where answers are expected to change? Do we document which tests failed in an initial run of the regression test suite ahead of creating a fresh baseline? Thanks! |
@MatthewPyle-NOAA We can confirm with pre-test. Usually on hera but down today. I let a test run on hercules now. I will attach log. We can go from there to reset bl_date.conf for new date. |
Thanks @jkbk2004 I'm running a pre-test on WCOSS now. |
@jkbk2004 @MatthewPyle-NOAA I'm also running RTs on Jet right now and will check the list of failures to make sure that all tests that fail use Thompson MP. I've also given @AndersJensen-NOAA the paths for the RT rundirs and baselines so that he can double check the results. |
As expected, all cases are changing. I am resetting with a new BL_DATE.
|
@jkbk2004 Good to see we had a consistent list of tests that failed. I'm creating new a new baseline for WCOSS. |
@grantfirl @MatthewPyle-NOAA All tests ran ok on hera/orion/hercules. |
@grantfirl @jkbk2004 All good with new baselines on wcoss. |
@grantfirl We can have time if you want to finish test on jet. Or it will be good as well, if you want to start merging ufs-community/ccpp-physics#179. |
I couldn't get any throughput on Jet and ran the rt.conf_rrfs on Hera with identical failures as listed. I'm OK to merge, although I think that @MatthewPyle-NOAA and @AndersJensen-NOAA would like to double-check that the changed results still look good scientifically. Anders has the paths of the baselines and RT output on Hera to make some comparisons. |
Sure! We can merge once @MatthewPyle-NOAA @AndersJensen-NOAA confirm the results. |
I will trust @AndersJensen-NOAA on confirming the results. |
@MatthewPyle-NOAA You mean to go for merging? |
@jkbk2004 I meant that once @AndersJensen-NOAA is good with it, I will be as well. |
@jkbk2004 @MatthewPyle-NOAA Via a separate email, @AndersJensen-NOAA confirms that the results change related to this PR is not detrimental to forecasts and is OK to merge. We can choose to merge this as-is, or we can combine #2170 into this one. Although that one is also expected to change results for any suites using MYNN PBL, those results changes are desired for the RRFS release. @jkbk2004 @MatthewPyle-NOAA Do we merge this as-is, or should we combine and maybe run another set of RTs for the combined changes? I'm fine with either decision. Thoughts? |
@grantfirl I'm fine either way - whatever seems easiest for you. |
I'm thinking that this was the only one that needed "extra" scientific validation and we can go ahead and merge since all the tests have been run and baselines updated. We'll either need new baselines for the combined PR or for the new PR by itself, so there will be more testing either way. It's probably easiest just to merge this and move on to the next one. |
@MatthewPyle-NOAA I can go ahead and start the merge. |
@jkbk2004 @MatthewPyle-NOAA Updated and ready to approve/merge |
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.
Looks good to me....
Commit Queue Requirements:
Description:
This PR is the first of many that will modernized, modularize, and streamline Thompson-Eidhammer microphysics.
This PR includes:
The addition of parameterized kind: REAL -> real(kind_phys)
Consistent indentation
Removal of GOTO statements
Add physical constant metadata needed by Thompson MP
Commit Message:
Priority:
Git Tracking
UFSWM:
Sub component Pull Requests:
UFSWM Blocking Dependencies:
Changes
Regression Test Changes (Please commit test_changes.list):
This PR only changes baselines due to changes in physical constants, which are now passed in and consistent with other physics schemes instead of redefining them.
Input data Changes:
Library Changes/Upgrades:
Testing Log: