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

Update NSSL microphysics with 3-moment option (and other updates) #110

Merged
merged 19 commits into from
Nov 6, 2023

Conversation

MicroTed
Copy link
Collaborator

@MicroTed MicroTed commented Sep 25, 2023

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:

  • Default graupel/hail fall speeds changed to use Milbrandt-Morrison parameters (icdx/icdxhl=6; can revert by setting icdx/icdxhl=3)
  • Increased resolution of gamma function lookup tables
  • New default graupel -> hail conversion based on wet growth diameter. A minimum conversion size also converts large graupel that is not in wet growth (e.g., some high plains storms) (Can revert by setting ihlcnh=1)
  • Improved representation of graupel sublimation at T > 0 (corrected a double-counting issue).
  • Can return maximum expected hail size (not used yet -- similar to nwp_diag in WRF)
  • Fixed a couple uninitialized work arrays
  • Fixed a problem where sedimentation did not work for k > 128 (i.e., if number of model levels exceeds 128, sedimentation only worked for k <= 128)

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

@MicroTed
Copy link
Collaborator Author

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?

@dustinswales
Copy link
Collaborator

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.
FYI. There are two ccpp-physics repositories, one for ufs-weather-model, one for the ccpp-SCM. As a developer, you only need to open your physics PRs into one of these. Then after your changes are merged, the code managers will incorporate your changes into the other repositories.

@MicroTed
Copy link
Collaborator Author

@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.

@MicroTed
Copy link
Collaborator Author

The noahmp error is now documented here: #114
I'm guessing that when compiled with optimization it either doesn't hit a 0/0 or else the NaN has no consequence.

@dustinswales
Copy link
Collaborator

@MicroTed Thanks for opening #114. I'm going to take a look at this today.

@MicroTed
Copy link
Collaborator Author

MicroTed commented Oct 2, 2023

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.
(Note that the glacier issue noted above only occurs in a debug test that is not part of the RT. It is unrelated to this PR as far as I can tell.)

@grantfirl
Copy link
Collaborator

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. (Note that the glacier issue noted above only occurs in a debug test that is not part of the RT. It is unrelated to this PR as far as I can tell.)

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?

@MicroTed
Copy link
Collaborator Author

MicroTed commented Oct 2, 2023

@grantfirl Yes, I think I figured out the upload thing. It's now in the PR description for ufs-weather-model. Thanks!

real :: cpi
real :: cap
real :: tfrcbw
real :: tfrcbi
real :: rovcp
real, public :: rdorv = 0.622
Copy link
Collaborator

@grantfirl grantfirl Oct 11, 2023

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?

Copy link
Collaborator Author

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

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.)
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

@grantfirl grantfirl left a 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.

@MicroTed
Copy link
Collaborator Author

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?

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.

@zach1221
Copy link

zach1221 commented Nov 6, 2023

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?

@grantfirl grantfirl merged commit c751a5a into ufs-community:ufs/dev Nov 6, 2023
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.

5 participants