-
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 NSSL microphysics with 3-moment option (and other updates) #110
Conversation
Also other various bug fixes etc.
Note to @grantfirl : I have a SCM update to provide the new variables: https://github.com/MicroTed/gmtb-scm/tree/feature/nssl-3moment (needs to be merged up to current main). SCM doesn't seem to work with the ufs-community fork, however, so I guess that can wait for now? |
@MicroTed I see you already opened PRs into the super modules (fv3atm, ufs-weather-model), which is all you need to do. |
@dustinswales Thanks, yes, I have draft PRs for FV3 and UFS while I work out the regression tests. I'm getting a weird failure with the rrfs_v1nssl dbg test with opnReqTest (intel compiler). Some FPE in noahmp_glacier_routines, which is a mystery at the moment. |
The noahmp error is now documented here: #114 |
The regression test on hera finally completed. The expected tests failed the baseline comparison, as noted in ufs-community/ufs-weather-model#1924 and all others passed. |
Could you please upload the tests/logs/RegressionTests_hera.log file either in the PR description or the comments of ufs-community/ufs-weather-model#1924? |
@grantfirl Yes, I think I figured out the upload thing. It's now in the PR description for ufs-weather-model. Thanks! |
physics/module_mp_nssl_2mom.F90
Outdated
real :: cpi | ||
real :: cap | ||
real :: tfrcbw | ||
real :: tfrcbi | ||
real :: rovcp | ||
real, public :: rdorv = 0.622 |
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.
I realize that there are still several physical constants being redefined in this module (which can introduce inconsistencies between this scheme and the host and other CCPP schemes), but why is rdorv being defined here when it is also set during the init phase? Just in case the constant intialization subroutine is skipped?
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.
Good question. I think that might be the only constant I added for CCPP the first time. It is only used in the driver routine (and only in the CCPP version -- not used in WRF). I'm not sure why I set it to public... probably should be private. I see there are a couple other constants that don't have initial values, and others that do, so that's not consistent. Would you prefer that all have initial values or none?
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.
If this same code is being run in a situation where the physical constants are not being set from the host (non-CCPP) AND in a situation where they are, then providing default initial values would be prudent. Otherwise, it may be misleading if someone is reading the code? Probably not a huge deal.
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.
I might as well set all the values for consistency while I'm making other minor changes.
physics/module_mp_nssl_2mom.F90
Outdated
g1palp = gmoi(i) + (gmoi(i+1) - gmoi(i))*del*dgami | ||
|
||
tmp = dn(ix,jy,kz)*an(ix,jy,kz,lh)/(hwdn*an(ix,jy,kz,lnh)) | ||
diam = (6.0*tmp/(3.14159))**(1./3.) |
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.
Seems a bit silly to keep redefining pi.
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.
Yeah, I can fix that. This was a new diagnostic subroutine aimed at WRF and isn't used in other models yet. I could block it out from the CCPP version completely for now, I suppose.
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.
Holy crow, the changes for module_mp_nssl_2mom.F90 were very hard to review and I doubt that I did a very thorough job, but I don't want to hold up your progress on this update. 5,522 changes in a file that is already >24K lines long is a lot. During the review, it appeared to me that there may be some code repetition that can be consolidated into subroutines? Also, at some point, it would be nice to go through the entire file and look for physical constants that are being redefined within. Lastly, I noticed some more write statements getting added that aren't controlled by a debug
-type flag which may annoy users if they are spamming standard out or log files.
The inclusion of the 3-moment option is fairly major, yes, and accounts for a lot of the changes. And I'm sure there are some repetitions. There shouldn't be any prints unless the model is going off the rails and blowing up (usually temperature going out of bounds). Debug prints should be otherwise turned off with hard-coded logic, partly because I'm lazy about removing prints that probably aren't needed any more. I just assume the compiler will prune those and not be impeded for optimization. |
…id not work for k > 128
Hello, @grantfirl @dustinswales regression testing is complete on UFS-WM #1924, and we are ready to begin the merging process. Can you please merge this ccpp-physics sub-pr for us? |
The main update is support for 3-moment rain, graupel, and hail (or just rain and graupel if the hail species is deactivated). This predicts the reflectivity moment (6th moment in diameter), which in turn predicts the shape parameter of the gamma function particle size distribution. A main advantage is better size sorting without the artificial graupel/hail breakup needed for the 2-moment scheme.
Other changes:
RT with ufs_model has been done on hera. Results change as expected for SDFs that use the NSSL scheme (FV3_rrfs_v1nssl and FV3_WoFS_v0). Weather model branch with FV3/ccpp update is
https://github.com/MicroTed/ufs-weather-model/tree/feature-nssl3m