Changes to RRTMGP. New ORTs by dustinswales · Pull Request #926 · ufs-community/ufs-weather-model

dustinswales · 2021-11-19T00:23:29Z

PR Checklist

Ths PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR. Please consult the ufs-weather-model wiki if you are unsure how to do this.
This PR has been tested using a branch which is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR
Results for one or more of the regression tests change and the reasons for the changes are understood and explained below.

Instructions: All subsequent sections of text should be filled in as appropriate.

The information provided below allows the code managers to understand the changes relevant to this PR, whether those changes are in the ufs-weather-model repository or in a subcomponent repository. Ufs-weather-model code managers will use the information provided to add any applicable labels, assign reviewers and place it in the Commit Queue. Once the PR is in the Commit Queue, it is the PR owner's responsiblity to keep the PR up-to-date with the develop branch of ufs-weather-model.

Description

This PR contains changes to FV3/ccpp-physics for the RRTMGP radiation scheme.
Coupled restart and decomposition operational regression tests uncovered some errors.

There were two errors.
-) Indexing bug in SW cloud-sampling routine. (dustinswales/ccpp-physics@0a545c6#diff-ab383f7b24008085d38216e27611d9db32967aa782caf00525886f1c46407665L102)
-) Revert bug introduced by NCAR/ccpp-physics#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.
There are changes to the answers with these changes.

Testing

How were these changes tested? What compilers / HPCs was it tested with? Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) Have regression tests and unit tests (utests) been run? On which platforms and with which compilers? (Note that unit tests can only be run on tier-1 platforms)

This PR contains new RTS.

Dependencies

waiting on Changes to FV3 for ccpp-physics #782 NOAA-EMC/ufsatm#428
waiting on RRTMGP cleanup from ORTs NCAR/ccpp-physics#782

junwang-noaa · 2021-11-19T14:56:29Z

+COMPILE | -DAPP=S2SW -DCCPP_SUITES=FV3_GFS_v16_coupled_nsstNoahmpUGWPv1,FV3_GFS_v16_coupled_rrtmgp                                | - wcoss_cray                            | fv3 |
 # Waves off
 RUN     | cpld_control_p7                                                                                                         | - wcoss_cray                            | fv3 |
+RUN     | cpld_control_p7_rrtmgp                                                                                                  | - wcoss_cray                            | fv3 |


Did the cpld p7 rrtmp pass ORT test including the threading test? We may need the ORT log file for the new feature test cpld_control_p7_rrtmgp .

Yes. 2thread, debug, decomposition, and restart passed.

MinsukJi-NOAA · 2021-11-19T15:20:08Z

A general comment/question:
According to discussions some time ago, I thought that adding new regression tests is done once a prototype is fully ready (e.g. p7), and if not the ORT (instead of RT) is used. Will these additional RTs be removed when p8 is ready?

dustinswales · 2021-11-19T15:32:43Z

A general comment/question: According to discussions some time ago, I thought that adding new regression tests is done once a prototype is fully ready (e.g. p7), and if not the ORT (instead of RT) is used. Will these additional RTs be removed when p8 is ready?

I'm not aware of what the correct process is to add a new test. I'll do whatever you suggest.

DeniseWorthen · 2021-11-19T16:12:00Z

The P7 test suite (control* p7) and cpld_control* p7) does not yet contain any new features planned for P8 (it does have modifications to existing P7 features, such as CA).

To add RRTMGP as a new P8 feature, in your branch you would add the new default values used by RRTMGP to the coupled part of default_vars (in the export_cpl () section). For example, there would be at a minimum

export DO_RRTMGP=.true.
...plus any other settings which may vary w/ resolution etc

Note that the cpld_control.nml.IN is used by both standalone and coupled P7 tests.

Then run the oRTs for both coupled and standalone P7 suites. For coupled, two of the tests need to be done without waves (debug and restart). @MinsukJi-NOAA can you provide more details?

Once we have confirmation that the new feature passes all the oRTs, the plan would be to commit that new feature as a new P8 test suite and drop the existing P7 suite w/ the exception of the bmark_p7 test. In other words, we would have cpld_bmark_p7 and cpld_bmark_p8 but all other P7 tests would be commited as P8 tests.

dustinswales · 2021-11-19T16:19:46Z

The P7 test suite (control* p7) and cpld_control* p7) does not yet contain any new features planned for P8 (it does have modifications to existing P7 features, such as CA).

RRTMGP is the new feature planned for p8.

To add RRTMGP as a new P8 feature, in your branch you would add the new default values used by RRTMGP to the coupled part of default_vars (in the export_cpl () section). For example, there would be at a minimum
export DO_RRTMGP=.true.
...plus any other settings which may vary w/ resolution etc
Note that the cpld_control.nml.IN is used by both standalone and coupled P7 tests.

Then run the oRTs for both coupled and standalone P7 suites. For coupled, two of the tests need to be done without waves (debug and restart). @MinsukJi-NOAA can you provide more details?

Once we have confirmation that the new feature passes all the oRTs, the plan would be to commit that new feature as a new P8 test suite and drop the existing P7 suite w/ the exception of the bmark_p7 test. In other words, we would have cpld_bmark_p7 and cpld_bmark_p8 but all other P7 tests would be commited as P8 tests.

I guess It's not clear what needs to be done to include new ORTs...
I followed the ORT presentation by @MinsukJi-NOAA from 10/20/2021.

DeniseWorthen · 2021-11-19T16:23:47Z

I understand about RRTMGP being a new P8 feature. My comment was only meant to explain that essentially the first "new feature" committed to the P7 test suite would in fact convert the P7 suite to P8.

MinsukJi-NOAA · 2021-11-19T16:31:42Z

I understand about RRTMGP being a new P8 feature. My comment was only meant to explain that essentially the first "new feature" committed to the P7 test suite would in fact convert the P7 suite to P8.

So, these new tests by @dustinswales will be used as the basis for the new suite of P8 regression tests? Then, I guess ORT (run by the script opnReqTest) is not needed for this PR.

DeniseWorthen · 2021-11-19T16:38:36Z

The idea is that at the time that the PR is made, the develop shows evidence that the oRTs have been run using the existing test suite (P7 say), but with their feature added.

At that point, we know we can add it to the commit Q

dustinswales · 2021-11-19T16:39:22Z

I understand about RRTMGP being a new P8 feature. My comment was only meant to explain that essentially the first "new feature" committed to the P7 test suite would in fact convert the P7 suite to P8.

So, these new tests by @dustinswales will be used as the basis for the new suite of P8 regression tests? Then, I guess ORT (run by the script opnReqTest) is not needed for this PR.

I can't speak to what should be the new basis of anything :)

I guess the question I have is, should I just create a single new coupled RT and add it to rt.conf (e.g https://github.com/ufs-community/ufs-weather-model/pull/926/files?authenticity_token=AVqgQaTSfQwaR8ph9mD6SRjEje8skGtDOJh6vw%2BtSqTOgqFQT1oNHElu1aSAP0gwZANX%2FGnSgtt3GFf6qcpJ1A%3D%3D&file-filters%5B%5D=.conf&file-filters%5B%5D=No+extension#diff-bca3e2e0c761ae3b013409651b01bd531fce420c95ce064b7253f677f81b787aL5).
Then the other tests (debug,decomp,threading,restart) will be exercised by calling opnReqTest?

MinsukJi-NOAA · 2021-11-19T18:12:24Z

I guess the question I have is, should I just create a single new coupled RT and add it to rt.conf (e.g https://github.com/ufs-community/ufs-weather-model/pull/926/files?authenticity_token=AVqgQaTSfQwaR8ph9mD6SRjEje8skGtDOJh6vw%2BtSqTOgqFQT1oNHElu1aSAP0gwZANX%2FGnSgtt3GFf6qcpJ1A%3D%3D&file-filters%5B%5D=.conf&file-filters%5B%5D=No+extension#diff-bca3e2e0c761ae3b013409651b01bd531fce420c95ce064b7253f677f81b787aL5). Then the other tests (debug,decomp,threading,restart) will be exercised by calling opnReqTest?

Yes, please make sure ./opnReqTest -n cpld_control_p7_rrtmgp -c thr,dcp,rst,dbg passes, and save the log.

Note that the old cpld_control_p7 (which I think you used) has changed to cpld_control_c96_p7. The new cpld_control_p7 includes the wave component.

Also, please create control_p7_rrtmgp off of control_p7 Then make sure ./opnReqTest -n control_p7_rrtmgp -c thr,dcp,rst,dbg all pases, save the log -- you can concatenate the two log files and commit to your branch.

dustinswales · 2021-11-19T18:16:32Z

Here is the log from "./opnReqTest -k -n cpld_control_p7_rrtmgp -c rst,dbg,thr,dcp"
/scratch2/BMC/ome/Dustin.Swales/UFS/ufs-weather-model-ad73e8a/tests/OpnReqTests_hera.intel.log

DeniseWorthen · 2021-11-30T15:10:57Z

@dustinswales We are going to start working on the commit for this PR. Please merge to the top of develop. Then, we need to make the following adjustments to your proposed additional tests:

Since this is the first P8 new feature to be added, when we commit this PR we will update the current P7 feature test suite to P8 (recognizing that additional features will be added as they become available for RT testing). The following changes will need to be made:

add the changes in your cpld_control_rrtmgp.nml.IN to the existing cpld_control.nml.IN. If any of the settings are resolution dependent, then those settings will need to be implemented as variables. Remove cpld_control_rrtmgp.nml.IN.
instead of adding additional _rrtmgp tests, add the changes to the existing _p7 tests. This needs to be done for both the cpld P7 test suite and the standalone (control) P7 test suite.
We will retain only the existing cpld_bmark_p7 test alongside the updated P8 tests. Once you've added your rrtmgp changes to the P7 test suites, they will need to be renamed as P8. Only for the cpld_bmark_p7 will you create additional tests. In this case, create a new cpld_bmark_p8 and cpld_bmark_mpi_p8. Drop the cpld_bmark_mpi_p7 test.
Update rt.conf to reflect the changes to the test names and the additional P8 bmark tests.

Please let us know if you have any questions. Thanks.

…r-model into HEAD

dustinswales · 2021-12-01T17:22:11Z

Hi All, I think I did all that you asked for, dustinswales@ee21b51 . New ORTs are for prototype 8. Anything else? Dustin

…

On Tue, Nov 30, 2021 at 8:11 AM Denise Worthen ***@***.***> wrote: @dustinswales <https://github.com/dustinswales> We are going to start working on the commit for this PR. Please merge to the top of develop. Then, we need to make the following adjustments to your proposed additional tests: Since this is the first P8 new feature to be added, when we commit this PR we will update the current P7 feature test suite to P8 (recognizing that additional features will be added as they become available for RT testing). The following changes will need to be made: 1. your cpld_control_rrtmgp.nml.IN should replace the existing cpld_control.nml.IN. If any of the settings are resolution dependent, then those settings will need to be implemented as variables. 2. instead of adding additional _rrtmgp tests, add the changes to the existing _p7 tests. This needs to be done for both the cpld P7 test suite and the standalone (control) P7 test suite. 3. We will retain only the existing cpld_bmark_p7 test alongside the updated P8 tests. Once you've added your rrtmgp changes to the P7 test suites, they will need to be renamed as P8. Only for the cpld_bmark_p7 will you create additional tests. In this case, create a new cpld_bmark_p8 and cpld_bmark_mpi_p8. Drop the cpld_bmark_mpi_p7 test. 4. Update rt.conf to reflect the changes to the test names and the additional P8 bmark tests. Please let us know if you have any questions. Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#926 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADKNQ2PKP5EBXE2PS5AWEH3UOTSQXANCNFSM5IK5MPWA> . 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>.

-- ---------------------------------------------------------------------------------- Dustin Swales Associate Scientist III CIRES /NOAA-ESRL (303)-497-7008 ***@***.*** ----------------------------------------------------------------------------------

DeniseWorthen · 2021-12-01T20:25:52Z

I'm still seeing additional _rrtmgp tests in your PR branch in addition to the p7 and p8 tests.

We need you to create the following tests (your control_p8 test looks correct):

control_p8
control_decomp_p8
control_2threads_p8
control_restart_p8
control_debug_p8

and remove the existing control_X_p7 tests. These files constitute the standalone test suite for P7.

Likewise for the coupled, we need you to create P8 versions of all P7 coupled tests and then remove all P7 coupled tests except for the cpld_bmark_p7 test. This will then be our coupled test suite for P8.

lisa-bengtsson · 2021-12-01T20:33:52Z

@DeniseWorthen is it possible for the code managers to create the necessary tests for P8 instead of putting this on Dustin?

DeniseWorthen · 2021-12-01T20:50:53Z

@lisa-bengtsson I can help work on the tests, but it will take at least a few days for me to find the time to do so.

dustinswales · 2021-12-01T21:12:04Z

I'm still seeing additional _rrtmgp tests in your PR branch in addition to the p7 and p8 tests.

We need you to create the following tests (your control_p8 test looks correct):
control_p8
control_decomp_p8
control_2threads_p8
control_restart_p8
control_debug_p8
and remove the existing control_X_p7 tests. These files constitute the standalone test suite for P7.

Likewise for the coupled, we need you to create P8 versions of all P7 coupled tests and then remove all P7 coupled tests except for the cpld_bmark_p7 test. This will then be our coupled test suite for P8.

@DeniseWorthen
I'm a bit confused.
The guidance recently provided to us developers was to add a new feature to an existing prototype using the operational regression tests. This was done, as cpld_control_p7_rrtmgp was part of the original PR.
What your are advising is different, and much more involved.
Should adding a new p8 test using this latest development be the developers responsibility?

DeniseWorthen · 2021-12-01T21:35:56Z

The oRTs is a set of tests which need to be run and shown to pass (with the new feature added) before we consider adding a PR the commit Q. It should be considered part of the PR process, not the commit process. If a feature breaks the oRTs, we cannot add the PR to the commit Q, regardless of how desirable the feature is.

Once a developer has shown that the new feature, added to the existing test suite (e.g., "P7"), does not break the operational requirements, then we can consider the PR ready to add to the commit Q.

We generally expect the developer to provide the test which exercises the feature. Since the feature you are adding belongs to the feature set planned for P8, it is more complicated than just adding a test for your own feature alone. In fact, if a feature is part of a prototype feature set, we do not maintain a separate test for that feature in isolation.

All that being said, as I indicated above, I will work on building the required tests but it will delay the commit until I have a chance to do so.

dustinswales · 2021-12-01T21:49:33Z

As part of this PR I provided a new ORT that exercised the new feature and did not break operational requirements.

All of the P8 business is beyond the scope of the PR. It's an added burden on the developer, especially the day before the code is to be merged.

DeniseWorthen · 2021-12-01T22:02:31Z

The fact that this commit would convert the P7 test suite to P8 was mentioned 12 days ago:

Once we have confirmation that the new feature passes all the oRTs, the plan would be to commit that new feature as a new P8 test suite and drop the existing P7 suite w/ the exception of the bmark_p7 test. In other words, we would have cpld_bmark_p7 and cpld_bmark_p8 but all other P7 tests would be commited as P8 tests.

I apologize if the process has not been clear or that you feel the code commit process is overly burdensome.

dustinswales · 2021-12-01T22:17:40Z

I changed the names of my coupled rrtmgp_p7 tests to p8, dustinswales@ee21b51
I added a new control test as well, but you want 4 more (dbg,thr,dcp,restart).

I had no problem changing the names, but adding all of these new tests for p8 is not my responsibility, nor should this PR be held up because of these p8 tests not being in place.

DeniseWorthen · 2021-12-02T14:35:33Z

@dustinswales I will be closing this PR and using PR #941 to make the changes required to update the tess.

* add namelist parameter sigmab_coldstart * fix for inline post when using RUC LSM * pass landsfcmdl to iSF_SURFACE_PHYSICS in post_fv3 * remove unnecessary change in io/post_fv3.F90 --------- Co-authored-by: Jili Dong <Jili.Dong@noaa.gov>

dustinswales added 8 commits November 11, 2021 23:39

Updated FV3/physics

8ead575

Updated .gitmodules

4cf8971

Updated .gitmodules

6f10c13

New ORT for RRTMGP.

818502b

Omission from previous commit

b7fc539

More RRTMGP coupled operational regression tests!

94c9d3d

Synced with origin

c795cbc

Updated submodules

b7ec3c3

junwang-noaa requested review from DeniseWorthen and MinsukJi-NOAA November 19, 2021 14:52

junwang-noaa reviewed Nov 19, 2021

View reviewed changes

Comment thread tests/parm/cpld_control_rrtmgp.nml.IN

junwang-noaa reviewed Nov 19, 2021

View reviewed changes

dustinswales added 7 commits November 30, 2021 15:40

Fixed typos in FV3/ccpp-physics.

6b7c869

Updated FV3.

78637e6

Updated CICE submodule.

7a5bf9d

Reverted rt.conf

cb86204

Added prototype-8 opertaional regression tests.

401cabc

Merge branch 'develop' of https://github.com/ufs-community/ufs-weathe…

351b85e

…r-model into HEAD

Added P8 control ORT.

1d0ca1b

Merge branch 'newdev' into add_GPorts2

ee21b51

Updated FV3.

d20454a

DeniseWorthen closed this Dec 2, 2021

Conversation

dustinswales commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Instructions: All subsequent sections of text should be filled in as appropriate.

Description

Testing

Dependencies

Uh oh!

Uh oh!

junwang-noaa Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dustinswales Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

MinsukJi-NOAA commented Nov 19, 2021

Uh oh!

dustinswales commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeniseWorthen commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dustinswales commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeniseWorthen commented Nov 19, 2021

Uh oh!

MinsukJi-NOAA commented Nov 19, 2021

Uh oh!

DeniseWorthen commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dustinswales commented Nov 19, 2021

Uh oh!

MinsukJi-NOAA commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dustinswales commented Nov 19, 2021

Uh oh!

DeniseWorthen commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dustinswales commented Dec 1, 2021 via email

Uh oh!

DeniseWorthen commented Dec 1, 2021

Uh oh!

lisa-bengtsson commented Dec 1, 2021

Uh oh!

DeniseWorthen commented Dec 1, 2021

Uh oh!

dustinswales commented Dec 1, 2021

Uh oh!

DeniseWorthen commented Dec 1, 2021

Uh oh!

dustinswales commented Dec 1, 2021

Uh oh!

DeniseWorthen commented Dec 1, 2021

Uh oh!

dustinswales commented Dec 1, 2021

Uh oh!

DeniseWorthen commented Dec 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dustinswales commented Nov 19, 2021 •

edited

Loading

junwang-noaa Nov 19, 2021 •

edited

Loading

dustinswales commented Nov 19, 2021 •

edited

Loading

DeniseWorthen commented Nov 19, 2021 •

edited

Loading

dustinswales commented Nov 19, 2021 •

edited

Loading

DeniseWorthen commented Nov 19, 2021 •

edited

Loading

MinsukJi-NOAA commented Nov 19, 2021 •

edited

Loading

DeniseWorthen commented Nov 30, 2021 •

edited

Loading