Skip to content

RRTMGP cleanup from ORTs#782

Merged
climbfuji merged 16 commits into
NCAR:mainfrom
dustinswales:cleanup_p8ORTs
Dec 9, 2021
Merged

RRTMGP cleanup from ORTs#782
climbfuji merged 16 commits into
NCAR:mainfrom
dustinswales:cleanup_p8ORTs

Conversation

@dustinswales
Copy link
Copy Markdown
Member

@dustinswales dustinswales commented Nov 18, 2021

This PR contains bug fixes to the RRTMGP radiation scheme discovered using the UFS operational regression tests.

There were two errors.
-) Indexing bug in SW cloud-sampling routine (dustinswales@0a545c6#diff-ab383f7b24008085d38216e27611d9db32967aa782caf00525886f1c46407665L102)

-) Revert bug introduced by #627. A little background. In this PR, the surface albedo calculation was moved out of the SW scheme and to the start of the radiation loop. In RRTMGP, the calculation of the solar-zenith angle was not moved out of the scheme along with the surface-albedo calculation. The SZA is an input to the subroutine to compute the surface-albedo. The fix is to call to coszmn() from GFS_rrtmgp_pre.F90, not GFS_rrtmgp_sw_pre.F90.

Additionally, there was some cleanup.

  • top_at_1, iSFC, and iTOA were moved out of the scheme files, calculated once in GFS_rrtmgp_pre.F90, defined as an Interstitial and passed around
  • Whitespace cleanup.
  • Optional argument cleanup.

Copy link
Copy Markdown
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a few changes in several files. Would you be able to expand the description of your PR, please, to help the reviewers? What issue is addressed, and how? We don't need a novel, just a few items that make it easier for us to review. Also, please list the associated fv3atm and ufs-weather-model PRs. Thanks!

I did give the PR a first quick look, thanks for cleaning up a lot of the trailing whitespaces.
I see you moved the calculation of top_at_1 moved from physics/GFS_rrtmgp_cloud_overlap_pre.F90 to physics/GFS_rrtmgp_pre.F90. Is this now the only place where it is calculated, and then sent to the other schemes as a host model/CCPP variable? II think so, but please confirm.

@climbfuji climbfuji requested a review from mzhangw November 19, 2021 14:19
Comment thread physics/GFS_rrtmgp_lw_post.F90 Outdated
Comment thread physics/GFS_rrtmgp_pre.meta
Comment thread physics/GFS_rrtmgp_sw_post.F90 Outdated
Comment thread physics/GFS_rrtmgp_sw_post.F90
@climbfuji
Copy link
Copy Markdown
Collaborator

@dustinswales This PR will be scheduled for commit soon, can you please address the comments from the code review so far? Thanks.

@dustinswales
Copy link
Copy Markdown
Member Author

I did give the PR a first quick look, thanks for cleaning up a lot of the trailing whitespaces. I see you moved the calculation of top_at_1 moved from physics/GFS_rrtmgp_cloud_overlap_pre.F90 to physics/GFS_rrtmgp_pre.F90. Is this now the only place where it is calculated, and then sent to the other schemes as a host model/CCPP variable? II think so, but please confirm.
Yes. I calculate this once and pass it into the scheme.

Copy link
Copy Markdown
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.

Nice work, @dustinswales. Thanks for the cleanup along with the other changes!

@grantfirl grantfirl mentioned this pull request Dec 6, 2021
@grantfirl
Copy link
Copy Markdown
Collaborator

I reviewed changes in rte-rrtmgp with commit 4bf8b44 and found nothing to change my previous approval of this PR from a CCPP point of view.

Copy link
Copy Markdown
Contributor

@SamuelTrahanNOAA SamuelTrahanNOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No complaints. I don't know the radiation code well, but at a technical level it looks good.

@climbfuji climbfuji merged commit cbc7e36 into NCAR:main Dec 9, 2021
@dustinswales dustinswales deleted the cleanup_p8ORTs branch December 9, 2021 23:20
hannahcbarnes pushed a commit to hannahcbarnes/ccpp-physics that referenced this pull request Aug 3, 2022
This PR contains changes to the RRTMGP code in the ccpp-physics PR#782.
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