-
Notifications
You must be signed in to change notification settings - Fork 149
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
Revisions to repair iovr=5 cloud overlap option #830
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.
The addition of iovr== 4/5 codes is straightforward.
To facilitate the future development and exercise this capability regularly , new RTs should be considered
Thanks @mzhangw . Both you and @mjiacono agree on a need to make sure iovr==4/5 are exercised in UFS RTs. I see that iovr==1 is in default_vars.sh and iovr==3 is exercised in several RTs, but I don't see any iovr==4/5 yet. I'll inquire about adding some. Maybe we can switch an existing RT to use these to save on the number of RTs. |
@Qingfu-Liu As codeowner of some of these files, would you please review these changes from @mjiacono ? |
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.
The changes appear to match what is described in #748. However, I'd like a review by another expert on the matter before I approve. Grant asked @Qingfu-Liu to review and I just asked @dustinswales since he commented a lot on #748.
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'm approving now that the code owner @Qingfu-Liu has approved, but I'm still hoping @dustinswales can review the changes since he commented on them a lot in the other issue.
All, thanks for the reviews. @mzhangw @mjiacono The new RTs that exercise iovr=4 and iovr=5 are OK with EMC code managers (see ufs-community/ufs-weather-model#1025). I'm rerunning RTs after addressing some reviewer comments in the ufs-weather-model PR, but this should be good to merge and is tentatively on the merge schedule for tomorrow. |
@@ -876,6 +876,19 @@ subroutine progcld1 & | |||
alpha(:,:) = 0. | |||
endif | |||
|
|||
! Revise alpha for exponential-random cloud overlap |
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.
This is exactly the same block of code, nine times, in the same file. In a future cleanup of the code, one could make this a separate subroutine to call from the various places.
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.
Now that Dom has opened this door, a word of explanation. My original vision for "get_alpha" was that it should define alpha for either iovr=4 (EXP) and iovr=5 (ER), and this was how I originally provided that subroutine. For some reason, get_alpha was later changed by someone to get_alpha_exp and simplified so that it only defined alpha for iovr=4. In the process the definition of alpha for iovr=5 was inadvertently dropped for RRTMG, which is the reason for the change provided by this PR. In the meantime, Dustin accounted for this discrepancy in RRTMGP by adding this code block after that model's call to get_alpha_exp. For this PR, I decided to apply Dustin's solution to RRTMG, which allowed this upgrade without also having to modify RRTMGP. I think it best at some point to revert to my original vision for get_alpha and build this block of code into that subroutine, which will have the advantage of providing the simplification that Dom mentions. However, it will mean passing iovr and cldfrac into get_alpha and then revising calls to it it accordingly.
I am working to clear up the code (finished the old version), and moving
all the calls to the subroutine "progcld*" to this module, and will move
this section of the code to the outside of those calls.
Qingfu
…On Thu, Jan 27, 2022 at 3:46 PM Dom Heinzeller ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In physics/radiation_clouds.f
<#830 (comment)>:
> @@ -876,6 +876,19 @@ subroutine progcld1 &
alpha(:,:) = 0.
endif
+ ! Revise alpha for exponential-random cloud overlap
This is exactly the same block of code, nine times, in the same file. In a
future cleanup of the code, one could make this a separate subroutine to
call from the various places.
—
Reply to this email directly, view it on GitHub
<#830 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGTS6UUDZ6VWIPEMCLCHIQTUYGVJFANCNFSM5MNQVL7Q>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I'll chime in and add that I've been saying for years that all of the "interstitials" surrounding radiation need a thorough reorganization/cleanup, especially in light of RRTMG and RRTMGP both being in the CCPP now. The current organization of interstitials was flawed from the beginning, IMO. |
^This |
@Qingfu-Liu When implementing GP I generalized much of the code in proglcd, which may be useful to you.
I'm happy to work with you to "mash this together" into some shared radiation infrastructure in the CCPP. |
I have asked Qingfu to write down his plan and share with relevant parties to discuss and come up with a better approach. |
This is what I am currently doing for the calculation of cloud radiation
properties for different microphysics (any comments and suggestions are
welcome):
1) added a new subroutine "radiation_clouds_prop" in the
"module_radiation_clouds".
This new subroutine will include all the subroutine calls to
following subroutines:
! internal accessable subroutines:
!
! 'progcld1' --- zhao/moorthi prognostic cloud scheme
!
! 'progcld2' --- inactive
!
! 'progcld3' --- zhao/moorthi prognostic cloud + pdfcld
!
! 'progcld4' --- GFDL-Lin cloud microphysics
!
! 'progcld4o' --- inactive
!
! 'progcld5' --- Ferrier-Aligo cloud microphysics
!
! 'progcld6' --- Thompson/wsm6 cloud microphysics (EMC) !
! 'progclduni' --- MG cloud microphysics
!
! --- GFDL cloud microphysics (EMC)
!
! --- Thompson + MYNN PBL (or GF convection)
!
! 'progcld_thompson' --- Thompson MG (added by G. Thompson)
The subroutine calls are removed from subroutine "GFS_rrtmg_pre_run".
By doing this, we will wrap all the calculations of the cloud radiation
properties in one module.
I have gone through those individual subroutines
(progcld1, progcld3, progcld4, ... ), I think it may be better to keep them
separate since they are for different microphysics schemes, although some
of them are very similar (still different). I will also move some common
code to the outside of those subroutines if possible, and add them to the
new subroutine.
Qingfu
…On Thu, Jan 27, 2022 at 5:29 PM Fanglin Yang ***@***.***> wrote:
I have asked Qingfu to write down his plan and share with relevant parties
to discuss and come up with a better approach.
—
Reply to this email directly, view it on GitHub
<#830 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGTS6UROZEZAT44ZSS4P6NLUYHBLBANCNFSM5MNQVL7Q>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
All credit to @mjiacono for this PR. All descriptions that follow are his. This replacement PR for #829 is for convenience only.
This PR addresses part 2 of issue #748 to activate the exponential-random cloud overlap method (iovr=5) in RRTMG.