search

NCAR / ccpp-physics

GFS physics for CCPP
Other
59 stars 146 forks source link

Radiation Improvements for prototype 8 #871

Closed dustinswales closed 2 years ago

dustinswales commented 2 years ago

This PR contains code changes to both the RRTMG and RRTMGP radiation schemes.

RRTMGP: Many of the changes are organizational, or added infrastructure for new features. This includes:

RRTMG: A request has been made by many physics developers and users to rewrite the cloud computation routines (progcldXXX) in the program radiation_clouds.f. Each microphysics scheme has one subroutine to calculate the cloud radiation properties, and those similar subroutines have many lines of common code. In the new code, we wrapped the calculation of cloud radiation properties in one single module “radiation_clouds.f” and moved all the common code from subroutines progcldXXX to the new subroutine “radiation_clouds_prop”. This subroutine can be called by RRTMG and RRTMGP. A single call to the subroutine “radiation_clouds_prop” can connect to the calculations of the cloud radiation properties for all the microphysics schemes. There are also many other changes in the following listed program based on the reviewer’s suggestions/comments. See https://docs.google.com/document/d/1y-0K3GS6ZwwKipwNNfBWE-k-TQaN3NwrgIbxRkdInYY/edit

grantfirl commented 2 years ago

@dustinswales I see that this starts from code in #858. I'm guessing that the merge conflicts stem from this. Assuming that @Qingfu-Liu updates his branch to resolve those conflicts, please update this branch afterward to do the same.

Also, I see that this PR has upstream PRs, but #858 does not. Is this PR supposed to include or be merged with #858, or what is the merge strategy?

dustinswales commented 2 years ago

@grantfirl These changes will go in after https://github.com/NCAR/ccpp-physics/pull/858 Whenever that goes in I'll make sure that everything is up-to-date in here.

grantfirl commented 2 years ago

This PR contains everything from the now-closed #862. @dustinswales Please give credit to @Qingfu-Liu for his changes in the description if you would.

grantfirl commented 2 years ago

@dustinswales OK, finished with review -- mostly minor quibbles. Nice job. Will approve when updated and conflicts resolved. I think that the biggest issue is probably resolving merge conflicts having to do with the recently-merged #761.

dustinswales commented 2 years ago

@grantfirl Thanks for the review. I made all of the changes you requested. The NSSL physics isn't going into GP, just G. @Qingfu-Liu You can still contribute code changes, I will just pull them in here.

Qingfu-Liu commented 2 years ago

Dustin, OK. I have a meeting now. I will look the code tomorrow. Qingfu

On Thu, Mar 10, 2022 at 3:32 PM dustinswales @.***> wrote:

@grantfirl https://github.com/grantfirl Thanks for the review. I made all of the changes you requested. The NSSL physics isn't going into GP, just G. @Qingfu-Liu https://github.com/Qingfu-Liu You can still contribute code changes, I will just pull them in here.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/871#issuecomment-1064477517, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGTS6USDP5EBL73UPEVE4FDU7JL5FANCNFSM5QHUHPSA . 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: @.***>

grantfirl commented 2 years ago

@dustinswales @Qingfu-Liu Tomorrow morning is the UFS code management meeting where the commit queue is planned for the next 2 weeks. What is your estimation of the "readiness" of this code. Is it ready enough to schedule it for next week?

dustinswales commented 2 years ago

@grantfirl The GP code is ready to roll.

Qingfu-Liu commented 2 years ago

@grantfirl https://github.com/grantfirl the code should be ready by the end of today. Qingfu

On Thu, Mar 10, 2022 at 5:51 PM dustinswales @.***> wrote:

@grantfirl https://github.com/grantfirl The GP code is ready to roll.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/871#issuecomment-1064588519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGTS6UWYUM42XM6C4MDDVMTU7J4GHANCNFSM5QHUHPSA . 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: @.***>

Qingfu-Liu commented 2 years ago

Hi Grant and Dustin, I just updated the code (radiation_clouds.f, GFS_rrtmg_pre.F90 and GFS_rrtmg_pre.meta) and resolved the code conflicts with the newest version (checked out on 03/10/2022), please update your PR 871. https://github.com/Qingfu-Liu/ccpp-physics/commit/71ab24d5a12a0e84d5802173c0526c9e1ed75e6c Thank you very much. Qingfu

On Fri, Mar 11, 2022 at 7:31 AM Qingfu Liu - NOAA Federal < @.***> wrote:

@grantfirl https://github.com/grantfirl the code should be ready by the end of today. Qingfu

On Thu, Mar 10, 2022 at 5:51 PM dustinswales @.***> wrote:

@grantfirl https://github.com/grantfirl The GP code is ready to roll.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/871#issuecomment-1064588519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGTS6UWYUM42XM6C4MDDVMTU7J4GHANCNFSM5QHUHPSA . 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: @.***>

Qingfu-Liu commented 2 years ago

Grant and Dustin, I forgot to mention that the merged code has been tested for the following suites and they produce binary identical results: cpld_control_p8 control control_p7_rrtmgp control_rrtmgp rrfs_v1nssl_nohailnoccn Qingfu

On Fri, Mar 11, 2022 at 12:06 PM Qingfu Liu - NOAA Federal < @.***> wrote:

Hi Grant and Dustin, I just updated the code (radiation_clouds.f, GFS_rrtmg_pre.F90 and GFS_rrtmg_pre.meta) and resolved the code conflicts with the newest version (checked out on 03/10/2022), please update your PR 871.

https://github.com/Qingfu-Liu/ccpp-physics/commit/71ab24d5a12a0e84d5802173c0526c9e1ed75e6c Thank you very much. Qingfu

On Fri, Mar 11, 2022 at 7:31 AM Qingfu Liu - NOAA Federal < @.***> wrote:

@grantfirl https://github.com/grantfirl the code should be ready by the end of today. Qingfu

On Thu, Mar 10, 2022 at 5:51 PM dustinswales @.***> wrote:

@grantfirl https://github.com/grantfirl The GP code is ready to roll.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/871#issuecomment-1064588519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGTS6UWYUM42XM6C4MDDVMTU7J4GHANCNFSM5QHUHPSA . 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: @.***>

grantfirl commented 2 years ago

Thanks @Qingfu-Liu . Once @dustinswales pulls in the changes from your feature/codeudpate2 branch, I have one more change to GFS_rrtmg_pre.F90 that needs to be put in here. It is a bugfix for the SPP code from @JeffBeck-NOAA. The bugfix should not affect any regression tests, and I just thought it makes more sense to put it in here (since GFS_rrtmg_pre.F90 is already being modified) than to put this bugfix in a different, unrelated PR.

grantfirl commented 2 years ago

Also, FYI, this is now scheduled in the UFS commit queue for next Friday, 3/18. Once this branch is updated with all changes, someone should run through all RTs on Hera. @dustinswales Do you typically do this? If not, I don't have a problem kicking these off and reporting results on https://github.com/ufs-community/ufs-weather-model/pull/1090

dustinswales commented 2 years ago

@grantfirl I just pulled in Qingfu's changes. I need to update FV3, for the nssl stuff, then I will run the RT's on Hera.

grantfirl commented 2 years ago

@dustinswales Before you run RTs, please merge this: https://github.com/dustinswales/ccpp-physics/pull/17 @JeffBeck-NOAA

JeffBeck-NOAA commented 2 years ago

Thank you for coordinating the SPP bug fix, @grantfirl, @dustinswales!

Qingfu-Liu commented 2 years ago

Sam, Great to know that intel bounds checking can catch the out-of-bounds when using dummy arguments.

Dustin, do you want me to change the code or you change the code. If I make the change, you need to copy the file. If you do it, the process may be simple. After all, it is a minor change. Qingfu

On Mon, Mar 21, 2022 at 12:29 PM Samuel Trahan (NOAA contractor) < @.***> wrote:

@.**** approved this pull request.

In physics/radiation_cloud_overlap.F90 https://github.com/NCAR/ccpp-physics/pull/871#discussion_r831295572:

 real(kind_phys), dimension(nCol), intent(in) :: &

dcorr_lgth ! Decorrelation length (km) real(kind_phys), dimension(nCol,nLay), intent(in) :: & dzlay !

  • real(kind_phys), dimension(nCol,nLay), intent(in) :: &
  • cld_frac

Dummy arguments should use implied shape arrays (dimension(:,:)) so the intel bounds checking will catch out-of-bounds accesses.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/ccpp-physics/pull/871#pullrequestreview-916012977, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGTS6UXA7WGLVHQX72KWOYDVBCPWZANCNFSM5QHUHPSA . You are receiving this because you were mentioned.Message ID: @.***>

climbfuji commented 2 years ago

@mzhangw @ChunxiZhang-NOAA You had previously commented on this PR, which is now approved by two of the CODEOWNERS. Can you please check if your comments have been addressed and approve as well? Thanks.