Add semi-Lagrangian sedimentation of rain to Thompson MP as an option by RuiyuSun · Pull Request #727 · NCAR/ccpp-physics

RuiyuSun · 2021-09-09T10:39:37Z

This is to add the semi-Lagrangian sedimentation of rain to Thompson MP as an option.

climbfuji

I only did a quick, high-level review and will look more closely over the next days. I am going to add additional reviewers, too.

yangfanglin · 2021-09-09T14:04:57Z

Dom, could you please add Songyou Hong and Shrinivas Moorthi as reviewers as well ? Thanks.

climbfuji · 2021-09-09T14:53:30Z

Dom, could you please add Songyou Hong and Shrinivas Moorthi as reviewers as well ? Thanks.

I added Moorthi. I do not have Songyou's GitHub ID. I will need that information and add him as a collaborator so that I can make him a reviewer.

dudhia · 2021-09-09T15:04:28Z

We have no evidence from WRF that Songyou has a github id. He may, but we have not seen it. Perhaps it is fine for Chunxi to act as a git contact?

…

On Thu, Sep 9, 2021 at 8:53 AM Dom Heinzeller ***@***.***> wrote: Dom, could you please add Songyou Hong and Shrinivas Moorthi as reviewers as well ? Thanks. I added Moorthi. I do not have Songyou's GitHub ID. I will need that information and add him as a collaborator so that I can make him a reviewer. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77HY62AZFA7L3BYDX7TUBDC7NANCNFSM5DW2NDVQ> . 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>.

Songyou184 · 2021-09-09T20:50:48Z

Can you see me ? Songyou

jianwenbao · 2021-09-09T20:57:47Z

If you assigned "songyou' as your ID, I do see you. -Bao

…

On 9/9/2021 2:50 PM, Songyou184 wrote: Can you see me ? Songyou — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC2LTIO5WZRMXDZALGWRVDDUBEM3HANCNFSM5DW2NDVQ>. 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>.

dudhia · 2021-09-09T21:05:59Z

Good, you have an id. That's what they wanted to know.

…

On Thu, Sep 9, 2021 at 2:50 PM Songyou184 ***@***.***> wrote: Can you see me ? Songyou — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEIZ77D57MRPIVW4F3NOEGTUBEM3HANCNFSM5DW2NDVQ> . 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>.

climbfuji · 2021-09-09T21:35:52Z

@Songyou184 Thanks for sharing! I am going to add you as collaborator, please watch out for an invite to "collaborate on ccpp-physcs". Once that is done, I can formally request a review from you, but of course you can look at the code changes beforehand.

climbfuji · 2021-09-22T18:42:42Z

@Songyou184 @gthompsnWRF did you have a chance to look at the suggested changes (Add semi-Lagrangian sedimentation of rain to Thompson MP as an option)?

Songyou184 · 2021-09-22T22:05:24Z

Hi Dom, I thought I did, but seems not committed correctly. Github is the new one to me... There are no major issues. Commented lines could be removed in subroutine nislfv_ppm,, also 100 continue. songyou

…

On Thu, Sep 23, 2021 at 3:43 AM Dom Heinzeller ***@***.***> wrote: @Songyou184 <https://github.com/Songyou184> @gthompsnWRF <https://github.com/gthompsnWRF> did you have a chance to look at the suggested changes (Add semi-Lagrangian sedimentation of rain to Thompson MP as an option)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVR3U6TT2GA75NZYKYUGEPTUDIPS7ANCNFSM5DW2NDVQ> . 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>.

SMoorthi-emc

I do not pretend to understand this code. However, as this code is based on SongYou's version and he has approved it it must be good to go.

RuiyuSun · 2021-09-23T16:08:48Z

Thank you, Songyou! Most of the comments and 100 continue line are removed.

…

On Wed, Sep 22, 2021 at 6:05 PM Songyou184 ***@***.***> wrote: Hi Dom, I thought I did, but seems not committed correctly. Github is the new one to me... There are no major issues. Commented lines could be removed in subroutine nislfv_ppm,, also 100 continue. songyou On Thu, Sep 23, 2021 at 3:43 AM Dom Heinzeller ***@***.***> wrote: > @Songyou184 <https://github.com/Songyou184> @gthompsnWRF > <https://github.com/gthompsnWRF> did you have a chance to look at the > suggested changes (Add semi-Lagrangian sedimentation of rain to Thompson MP > as an option)? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#727 (comment)>, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AVR3U6TT2GA75NZYKYUGEPTUDIPS7ANCNFSM5DW2NDVQ > > . > 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 authored the thread. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFOCLAG5FPVIE3Q3HQCOATUDJHK7ANCNFSM5DW2NDVQ> . 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>.

-- Ruiyu Sun, PhD *IMSG* at NOAA/NWS/NCEP/EMC 5830 University Research Ct., Rm. 2097 College Park, MD 20740 ***@***.*** ***@***.***> 301-683-3787

climbfuji · 2021-10-08T14:47:38Z

@gthompsnWRF We are hoping to merge this, if acceptable, on 10/15. Would you mind taking a look and approve/suggest changes/reject? Thanks in advance!

gthompsnWRF

I made many change requests, but most of it is related to coding standards, particularly removal of unused variables and the declaration for INTENT for subroutine arguments to protect against troubles.

gthompsnWRF · 2021-10-09T14:30:10Z

+            qrten(k) = qrten(k) + (rr(k) - rr_tmp(k))/rho(k)/dt
+            nrten(k) = nrten(k) + (nr(k) - nr_tmp(k))/rho(k)/dt
+          enddo
+          pptrain = pptrain + precip


Since I believe using a minimum mixing ratio of R1, the precip variable may have a tiny amount, which is why I added in my code a line like this...

if (rr(kts).gt.R1*10.) & pptrain = pptrain + (new rain amount)

therefore please add it.

'if (rr(kts).gt.R1*10.) &' is added. @Songyou184 Could you confirm this? Thanks!

Songyou184 · 2021-10-11T16:32:48Z

Greg and Ruiyu, IFIX can be replaced with INT. Regarding the goto statement in the SEMI_PPM routine, there was a CYCLE I_LOOP in the code that I sent ( commented ), which could be replaced. Adding SEDI_SEMI for Graupel should be straightforward, I believe. For DECFL, it is a ratio of the number of CFL time step for Eulerian scheme. It is set as 8 for safety, and could be 10, based on the idealized case experiment in Junag and Hong. This implementation differs from that in WRF WSM code. Instead of refining the Vt inside of SEMI largrangian scheme, we decided to use the Vt at the departure points, so it is now a Non-iterative forward SEMI-Lagrangian. Realizing the fact that Semi-Lagrangian still needs a CFL condition, we defined the DeCFL outside of Semi sedimentation, which is the number of subtime steps over the Eulerian scheme. For SEMI-Lagrangian, the number of iteration is reduced by the factor of DECFL. Unused variables and statements that Greg pointed out in SEMI_PPM are for refining the Vt in the WRF code, which could be removed in the new code, Thanks for the feedbacks... songyou

…

On Mon, Oct 11, 2021 at 7:00 AM RuiyuSun ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In physics/module_mp_thompson.F90 <#727 (comment)>: > @@ -3916,6 +3929,54 @@ subroutine mp_thompson (qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, & if (rr(kts).gt.R1*10.) & pptrain = pptrain + sed_r(kts)*DT*onstep(1) enddo + else !if(.not. sedi_semi) + niter = 1 + dtcfl = dt + if(sedi_semi_decfl) then + niter = ifix(nstep/decfl) + 1 @Songyou184 <https://github.com/Songyou184> Could you confirm the ifix change and answer the question about defcl? thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVR3U6X6SFHWBLQ2WF24YVTUGLNX3ANCNFSM5DW2NDVQ> . 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>.

RuiyuSun · 2021-10-12T13:12:02Z

Greg, The commit was pushed. Thank you for the detailed review.

…

On Tue, Oct 12, 2021 at 8:15 AM gthompsnWRF ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In physics/module_mp_thompson.F90 <#727 (comment)>: > REAL, DIMENSION(kts:kte):: temp, pres, qv REAL, DIMENSION(kts:kte):: rc, ri, rr, rs, rg, ni, nr, nc, nwfa, nifa + REAL, DIMENSION(kts:kte):: rr_tmp,vtrk_tmp,nr_tmp,vtnrk_tmp + REAL, DIMENSION(kts:kte):: ri_tmp,vtik_tmp,ni_tmp,vtnik_tmp Arrays for ice, snow, and graupel are removed. Ruiyu, Please push your commits back to GitHub so I can see the changes to the code. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFOCLCPCYF4VUIYICUND3LUGQRFDANCNFSM5DW2NDVQ> . 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>.

-- Ruiyu Sun, PhD *IMSG* at NOAA/NWS/NCEP/EMC 5830 University Research Ct., Rm. 2097 College Park, MD 20740 ***@***.*** ***@***.***> 301-683-3787

Songyou184 · 2021-10-12T20:47:23Z

In semi-Lagrangian, we don't need the additional vertical laye (kte+1)r. Thus vtrk_tmp, vtrnk_tmp (kts:kte) are correct.

Songyou184 · 2021-10-12T20:51:22Z

Ruiyu, I agree to all the changes.... except for the additional array for vtrk_tmp and vtrnk_tmp. songyou

…

On Mon, Oct 11, 2021 at 8:08 PM RuiyuSun ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In physics/module_mp_thompson.F90 <#727 (comment)>: > + dtcfl = dt/niter + endif + do n = 1, niter + rr_tmp(:) = rr(:) + nr_tmp(:) = nr(:) + do k = 1,kte + vtrk_tmp(k) = vtrk(k) + vtnrk_tmp(k) = vtnrk(k) + enddo + call nislfv_rain_ppm(kte,rho,rhof,temp,dzq,vtrk_tmp,rr,precip,dtcfl,1,0,R1) + call nislfv_rain_ppm(kte,rho,rhof,temp,dzq,vtnrk_tmp,nr,vtr,dtcfl,1,0,R2) + do k = kts, kte + qrten(k) = qrten(k) + (rr(k) - rr_tmp(k))/rho(k)/dt + nrten(k) = nrten(k) + (nr(k) - nr_tmp(k))/rho(k)/dt + enddo + pptrain = pptrain + precip 'if (rr(kts).gt.R1*10.) &' is added. @Songyou184 <https://github.com/Songyou184> Could you confirm this? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVR3U6TU44AGDCPRG6DMOXTUGOKCXANCNFSM5DW2NDVQ> . 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>.

RuiyuSun · 2021-10-13T02:35:43Z

Songyou, Thank you for answering the question. The additional arrays vtrk_tmp and vtrnk_tmp are removed. The commit has been pushed. Ruiyu

…

On Tue, Oct 12, 2021 at 4:51 PM Songyou184 ***@***.***> wrote: Ruiyu, I agree to all the changes.... except for the additional array for vtrk_tmp and vtrnk_tmp. songyou On Mon, Oct 11, 2021 at 8:08 PM RuiyuSun ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In physics/module_mp_thompson.F90 > <#727 (comment)>: > > > + dtcfl = dt/niter > + endif > + do n = 1, niter > + rr_tmp(:) = rr(:) > + nr_tmp(:) = nr(:) > + do k = 1,kte > + vtrk_tmp(k) = vtrk(k) > + vtnrk_tmp(k) = vtnrk(k) > + enddo > + call nislfv_rain_ppm(kte,rho,rhof,temp,dzq,vtrk_tmp,rr,precip,dtcfl,1,0,R1) > + call nislfv_rain_ppm(kte,rho,rhof,temp,dzq,vtnrk_tmp,nr,vtr,dtcfl,1,0,R2) > + do k = kts, kte > + qrten(k) = qrten(k) + (rr(k) - rr_tmp(k))/rho(k)/dt > + nrten(k) = nrten(k) + (nr(k) - nr_tmp(k))/rho(k)/dt > + enddo > + pptrain = pptrain + precip > > 'if (rr(kts).gt.R1*10.) &' is added. @Songyou184 > <https://github.com/Songyou184> Could you confirm this? Thanks! > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#727 (comment)>, or > unsubscribe > < https://github.com/notifications/unsubscribe-auth/AVR3U6TU44AGDCPRG6DMOXTUGOKCXANCNFSM5DW2NDVQ > > . > 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 authored the thread. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFOCLHMQTFOMRZER6GUTRTUGSNVLANCNFSM5DW2NDVQ> . 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>.

-- Ruiyu Sun, PhD *IMSG* at NOAA/NWS/NCEP/EMC 5830 University Research Ct., Rm. 2097 College Park, MD 20740 ***@***.*** ***@***.***> 301-683-3787

RuiyuSun · 2021-10-13T12:56:09Z

Songyou, If you think the code is fine could you approve? Thanks Ruiyu On Tue, Oct 12, 2021 at 10:35 PM Ruiyu Sun - NOAA Affiliate < ***@***.***> wrote:

…

Songyou, Thank you for answering the question. The additional arrays vtrk_tmp and vtrnk_tmp are removed. The commit has been pushed. Ruiyu On Tue, Oct 12, 2021 at 4:51 PM Songyou184 ***@***.***> wrote: > Ruiyu, > I agree to all the changes.... except for the additional array for > vtrk_tmp > and vtrnk_tmp. > songyou > > > On Mon, Oct 11, 2021 at 8:08 PM RuiyuSun ***@***.***> wrote: > > > ***@***.**** commented on this pull request. > > ------------------------------ > > > > In physics/module_mp_thompson.F90 > > <#727 (comment)>: > > > > > + dtcfl = dt/niter > > + endif > > + do n = 1, niter > > + rr_tmp(:) = rr(:) > > + nr_tmp(:) = nr(:) > > + do k = 1,kte > > + vtrk_tmp(k) = vtrk(k) > > + vtnrk_tmp(k) = vtnrk(k) > > + enddo > > + call > nislfv_rain_ppm(kte,rho,rhof,temp,dzq,vtrk_tmp,rr,precip,dtcfl,1,0,R1) > > + call > nislfv_rain_ppm(kte,rho,rhof,temp,dzq,vtnrk_tmp,nr,vtr,dtcfl,1,0,R2) > > + do k = kts, kte > > + qrten(k) = qrten(k) + (rr(k) - rr_tmp(k))/rho(k)/dt > > + nrten(k) = nrten(k) + (nr(k) - nr_tmp(k))/rho(k)/dt > > + enddo > > + pptrain = pptrain + precip > > > > 'if (rr(kts).gt.R1*10.) &' is added. @Songyou184 > > <https://github.com/Songyou184> Could you confirm this? Thanks! > > > > — > > You are receiving this because you were mentioned. > > Reply to this email directly, view it on GitHub > > <#727 (comment)>, > or > > unsubscribe > > < > https://github.com/notifications/unsubscribe-auth/AVR3U6TU44AGDCPRG6DMOXTUGOKCXANCNFSM5DW2NDVQ > > > > . > > 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 authored the thread. > Reply to this email directly, view it on GitHub > <#727 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AKFOCLHMQTFOMRZER6GUTRTUGSNVLANCNFSM5DW2NDVQ> > . > 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>. > > -- Ruiyu Sun, PhD *IMSG* at NOAA/NWS/NCEP/EMC 5830 University Research Ct., Rm. 2097 College Park, MD 20740 ***@***.*** ***@***.***> 301-683-3787

-- Ruiyu Sun, PhD *IMSG* at NOAA/NWS/NCEP/EMC 5830 University Research Ct., Rm. 2097 College Park, MD 20740 ***@***.*** ***@***.***> 301-683-3787

RuiyuSun · 2021-10-13T15:26:22Z

Greg, Do you have any other questions or suggestions? Thanks! Ruiyu

…

On Tue, Oct 12, 2021 at 8:15 AM gthompsnWRF ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In physics/module_mp_thompson.F90 <#727 (comment)>: > REAL, DIMENSION(kts:kte):: temp, pres, qv REAL, DIMENSION(kts:kte):: rc, ri, rr, rs, rg, ni, nr, nc, nwfa, nifa + REAL, DIMENSION(kts:kte):: rr_tmp,vtrk_tmp,nr_tmp,vtnrk_tmp + REAL, DIMENSION(kts:kte):: ri_tmp,vtik_tmp,ni_tmp,vtnik_tmp Arrays for ice, snow, and graupel are removed. Ruiyu, Please push your commits back to GitHub so I can see the changes to the code. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFOCLCPCYF4VUIYICUND3LUGQRFDANCNFSM5DW2NDVQ> . 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>.

-- Ruiyu Sun, PhD *IMSG* at NOAA/NWS/NCEP/EMC 5830 University Research Ct., Rm. 2097 College Park, MD 20740 ***@***.*** ***@***.***> 301-683-3787

climbfuji · 2021-10-14T16:04:00Z

@Songyou184 @RuiyuSun @gthompsnWRF I am on leave but trying to organize the commit of this PR. I see that three items from the code review have not been addressed yet:

@SMoorthi-emc saying that proper indentation should be used
@climbfuji saying that the order of arguments in the call to mp_thompson_run in mp_thompson.F90 should be different (in the Fortran subroutine call as well as in the metadata)
@gthompsnWRF asking about renaming the variable precip in line 1911 of module_mp_thompson.F90

Is that all that is missing? Can this be addressed please? Thanks!

gthompsnWRF

Only remaining request is to clean up the left over comment/question to Songyou.

While using the name precip as a variable could be confusing, I do not have a specific solution that I can suggest.

gthompsnWRF · 2021-10-14T16:17:40Z

+            qrten(k) = qrten(k) + (rr(k) - rr_tmp(k))/rho(k)/dt
+            nrten(k) = nrten(k) + (nr(k) - nr_tmp(k))/rho(k)/dt
+          enddo
+          if (rr(kts).gt.R1*10.) & !Songyou: is this needed? 


Upon removing this question to Songyou, I am otherwise approving this PR.

Ok, this is the only thing left.

The comment is removed.

Songyou184 · 2021-10-14T17:01:35Z

if (rr(kts).gt.R1*10.) & !Songyou: is this needed? No, we do not need this condition since rainfall at sfc is the summation of falling rainfall below the surface in SEMI-LAG. songyou

…

On Thu, Oct 14, 2021 at 10:56 AM Dom Heinzeller ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In physics/module_mp_thompson.F90 <#727 (comment)>: > + niter = 1 + dtcfl = dt + if(sedi_semi_decfl) then + niter = int(nstep/decfl) + 1 + dtcfl = dt/niter + endif + do n = 1, niter + rr_tmp(:) = rr(:) + nr_tmp(:) = nr(:) + call nislfv_rain_ppm(kte,dzq,vtrk,rr,precip,dtcfl,R1) + call nislfv_rain_ppm(kte,dzq,vtnrk,nr,vtr,dtcfl,R2) + do k = kts, kte + qrten(k) = qrten(k) + (rr(k) - rr_tmp(k))/rho(k)/dt + nrten(k) = nrten(k) + (nr(k) - nr_tmp(k))/rho(k)/dt + enddo + if (rr(kts).gt.R1*10.) & !Songyou: is this needed? Ok, this is the only thing left. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVR3U6XKUZTIK7IYTYTUW5DUG4DRXANCNFSM5DW2NDVQ> . 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>.

RuiyuSun · 2021-10-14T17:17:08Z

if (rr(kts).gt.R110.) & !Songyou: is this needed? No, we do not need this condition since rainfall at sfc is the summation of falling rainfall below the surface in SEMI-LAG. songyou
…
On Thu, Oct 14, 2021 at 10:56 AM Dom Heinzeller @.> wrote: @.** commented on this pull request. ------------------------------ In physics/module_mp_thompson.F90 <#727 (comment)>: > + niter = 1 + dtcfl = dt + if(sedi_semi_decfl) then + niter = int(nstep/decfl) + 1 + dtcfl = dt/niter + endif + do n = 1, niter + rr_tmp(:) = rr(:) + nr_tmp(:) = nr(:) + call nislfv_rain_ppm(kte,dzq,vtrk,rr,precip,dtcfl,R1) + call nislfv_rain_ppm(kte,dzq,vtnrk,nr,vtr,dtcfl,R2) + do k = kts, kte + qrten(k) = qrten(k) + (rr(k) - rr_tmp(k))/rho(k)/dt + nrten(k) = nrten(k) + (nr(k) - nr_tmp(k))/rho(k)/dt + enddo + if (rr(kts).gt.R1*10.) & !Songyou: is this needed? Ok, this is the only thing left. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVR3U6XKUZTIK7IYTYTUW5DUG4DRXANCNFSM5DW2NDVQ . 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.

The if condition is removed.

climbfuji

I am ok with this, thanks for making all the changes.

grantfirl · 2021-10-18T16:20:57Z

+          call nislfv_rain_ppm(kte,dzq,vtrk,rr,rainsfc,dtcfl,R1)
+          call nislfv_rain_ppm(kte,dzq,vtnrk,nr,vtr,dtcfl,R2)
+          do k = kts, kte
+            qrten(k) = qrten(k) + (rr(k) - rr_tmp(k))/rho(k)/dt


If rho(k) is constant through the n iterations, since division is slower than multiplication, it would be more computationally efficient to define a variable of 1/(rho*dt) to multiply here instead of the repeated division.

grantfirl

Looks fine from a CCPP point-of-view. There is probably room for some computational speed-up by paying close attention to divisions (and using multiplication times the reciprocal when possible), but perhaps at the expense of more temporary memory usage.

SMoorthi-emc · 2021-10-18T16:42:40Z

It is good practice to minimize division.

…

On Mon, Oct 18, 2021 at 12:24 PM grantfirl ***@***.***> wrote: ***@***.**** approved this pull request. Looks fine from a CCPP point-of-view. There is probably room for some computational speed-up by paying close attention to divisions (and using multiplication times the reciprocal when possible), but perhaps at the expense of more temporary memory usage. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#727 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALLVRYQLOVOA3UAOQDTQ6Y3UHRC3NANCNFSM5DW2NDVQ> . 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>.

-- Dr. Shrinivas Moorthi Research Meteorologist Modeling and Data Assimilation Branch Environmental Modeling Center / National Centers for Environmental Prediction 5830 University Research Court - (W/NP23), College Park MD 20740 USA Tel: (301)683-3718 e-mail: ***@***.*** Phone: (301) 683-3718 Fax: (301) 683-3718

* Code update for HR4_roughness

Add semi-Lagrangian sedimentation of rain to Thompson MP as an option

5f28eff

RuiyuSun requested review from JulieSchramm, climbfuji, grantfirl and llpcarson as code owners September 9, 2021 10:39

climbfuji reviewed Sep 9, 2021

View reviewed changes

Comment thread physics/mp_thompson.F90 Outdated

Comment thread physics/mp_thompson.meta

climbfuji requested a review from gthompsnWRF September 9, 2021 13:04

climbfuji requested a review from SMoorthi-emc September 9, 2021 14:52

climbfuji requested a review from Songyou184 September 10, 2021 13:36

SMoorthi-emc reviewed Sep 23, 2021

View reviewed changes

Comment thread physics/module_mp_thompson.F90 Outdated

SMoorthi-emc approved these changes Sep 23, 2021

View reviewed changes

remove comments in semi-lagrangian routine in the module_mp_thompson.F90

fc0eb08

RuiyuSun mentioned this pull request Oct 8, 2021

adding semi-lagrangian sedimentation of rain in Thompson MP as an option ufs-community/ufs-weather-model#863

Merged

14 tasks

gthompsnWRF reviewed Oct 9, 2021

View reviewed changes

make changes in response to reviews

abb367d

remove variables that are not used

f8f4716

gthompsnWRF approved these changes Oct 14, 2021

View reviewed changes

added a few minor changes in style

1254556

junwang-noaa reviewed Oct 14, 2021

View reviewed changes

Comment thread physics/mp_thompson.meta Outdated

RuiyuSun added 2 commits October 14, 2021 16:50

correct a typo

b6d102f

remove a comment

b381861

remove an if condition

40afbad

climbfuji approved these changes Oct 14, 2021

View reviewed changes

Merge remote-tracking branch 'ncar/main' into Thompson_semi_lag_rain

7327d21

grantfirl reviewed Oct 18, 2021

View reviewed changes

grantfirl approved these changes Oct 18, 2021

View reviewed changes

RuiyuSun added 2 commits October 25, 2021 12:37

Merge remote-tracking branch 'ncar/main' into Thompson_semi_lag_rain

7a3e0a5

Merge remote-tracking branch 'ncar/main' into Thompson_semi_lag_rain

bf92aa6

climbfuji merged commit 530f597 into NCAR:main Oct 28, 2021

RuiyuSun mentioned this pull request Dec 30, 2021

adding semi-Lagrangian sedimentation of rain into Thompson MP #724

Closed

JohanaRomeroAlvarez pushed a commit to JohanaRomeroAlvarez/ccpp-physics that referenced this pull request Sep 8, 2025

Code update for HR4_roughness (NCAR#727)

bd38c56

* Code update for HR4_roughness

Conversation

RuiyuSun commented Sep 9, 2021

Uh oh!

climbfuji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yangfanglin commented Sep 9, 2021

Uh oh!

climbfuji commented Sep 9, 2021

Uh oh!

dudhia commented Sep 9, 2021 via email

Uh oh!

Songyou184 commented Sep 9, 2021

Uh oh!

jianwenbao commented Sep 9, 2021 via email

Uh oh!

dudhia commented Sep 9, 2021 via email

Uh oh!

climbfuji commented Sep 9, 2021

Uh oh!

climbfuji commented Sep 22, 2021

Uh oh!

Songyou184 commented Sep 22, 2021 via email

Uh oh!

Uh oh!

SMoorthi-emc left a comment

Choose a reason for hiding this comment

Uh oh!

RuiyuSun commented Sep 23, 2021 via email

Uh oh!

climbfuji commented Oct 8, 2021

Uh oh!

gthompsnWRF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gthompsnWRF Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

RuiyuSun Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

Songyou184 commented Oct 11, 2021 via email

Uh oh!

RuiyuSun commented Oct 12, 2021 via email

Uh oh!

Songyou184 commented Oct 12, 2021

Uh oh!

Songyou184 commented Oct 12, 2021 via email

Uh oh!

RuiyuSun commented Oct 13, 2021 via email

Uh oh!

RuiyuSun commented Oct 13, 2021 via email

Uh oh!

RuiyuSun commented Oct 13, 2021 via email

Uh oh!

climbfuji commented Oct 14, 2021

Uh oh!

gthompsnWRF left a comment

Choose a reason for hiding this comment

Uh oh!

gthompsnWRF Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

climbfuji Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

RuiyuSun Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!