-
Notifications
You must be signed in to change notification settings - Fork 147
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
Miscellaneous Bugfixes from NRL #954
Conversation
…cessary; include NRL's lextop bugfix
@JongilHan66 Please review the changes in SAS and see the description of the problem solved by @matusmartini in the PR description. |
fixes #955 |
|
Sure, @JongilHan66, I can make those changes. @matusmartini Do you think this modification will suit your purposes? |
@JongilHan66 @matusmartini We could also potentially add a flag to control whether this overshooting limiter is active. Thoughts? |
Since the overshooting layers are smaller than the cloud depth most of time, I don't think adding a flag for the overshooting limiter is necessary. |
@JongilHan66 Can you verify that 1944081 is what you had in mind please? |
Yes, your modification is what I had in mind. |
This looks good! @grantfirl Thank you! @JongilHan66 |
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.
@grantfirl
This all looks good to me, although I'm not familiar with the indexing in the sas convection, but all the radiation stuff looks good. One small comment on radiation_astronomy.
I've looked into the overshoot modification and found a problem. The line, 'tem1=zi(i,ktcon1(i))-zi(i,ktcon(i))', should be changed to 'tem1=zi(i,k)-zi(i,ktcon(i))' in both samfdeepcnv & sascnvn. |
Inboth samfdeepcnv & sascnvn, please modify them to more compact form and overshooting not to be deeper than half of the actual cloud as:
!NRL MNM: Limit overshooting not to be deeper than the actual cloud ==>
!NRL MNM: Limit overshooting not to be deeper than half of the actual cloud Thank you. |
@JongilHan66 I think that I addressed your comments in 4cd35ab if you'd like to verify. |
@grantfirl I confirm the changes are correct and what I meant. |
All of these changes originally came from #772. These changes were split from the single precision work since they were unrelated, but they're still relevant and should be merged since they fix several bugs. @michalakes, @areinecke, and @matusmartini deserve the credit. My work was only to update it to the latest main and test it in FV3/SCM.
The first 4 commits do not change any UFS RT results. They represent the following:
The last commit introduces a change to SAS-based deep convection schemes to avoid negative qv. Any UFS RTs that use samfdeepcnv or sascnvn will need new baselines if this is merged. The following description is from @matusmartini: