Dsfcalc fix #727

ADCollard · 2024-03-22T21:41:30Z

Tiny fix to allow modelling of sub-fov variability for NOAA-21 ATMS.

Fixes #725.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Stand alone run. Does not change results unless dfscalc is set to 1 for NOAA-21

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
New and existing tests pass with my changes
Any dependent changes have been merged and published

RussTreadon-NOAA

Looks good to me. Thank you @ADCollard

RussTreadon-NOAA · 2024-03-24T22:41:28Z

@emilyhcliu , @xincjin-NOAA , @XuanliLi-NOAA , or @azadeh-gh : Do any of you have time to review this PR? We need to peer reviews and approvals.

RussTreadon-NOAA · 2024-03-24T22:46:34Z

@ADCollard : With this change we can set dsfcalc to 1 for atm_n21 on line 184 of regression/regression_namelists.sh. Would you please add this change to your branch.

xincjin-NOAA · 2024-03-25T03:36:29Z

@RussTreadon-NOAA I am not in the reviewer list.

RussTreadon-NOAA · 2024-03-25T10:45:57Z

@xincjin-NOAA , thank you for your reply. You're ahead of me.

We are trying to process GSI PRs in a more agile manner. I saw this PR yesterday and self-assigned myself as the GSI Handling Review team member shepherding this PR. Hence my review and request.

We need at least two peer reviews and approvals. Andrew may have reached out to potential peer reviewers. I don't know. I decided to be proactive and reach out four developers, including you, to see if any of you have time to review this PR. I don't assign reviewers without asking first. We are all busy. I only assign reviewers after I receive confirmation that (a) they can review the PR and (b) do so in a timely manner.

XuanliLi-NOAA · 2024-03-25T14:40:01Z

I'm not very familiar with this part of GSI, but I'd be happy to review the PR if others are unavailable. Do I just need to run the ctest following the GSI wiki?

ADCollard · 2024-03-25T15:01:17Z

@ADCollard : With this change we can set dsfcalc to 1 for atm_n21 on line 184 of regression/regression_namelists.sh. Would you please add this change to your branch.

@RussTreadon-NOAA I am just running the regression tests for this, but this change should cause them to fail. I will document differences.

RussTreadon-NOAA · 2024-03-25T15:01:26Z

@XuanliLi-NOAA: No, you do not need to run ctests as a peer reviewer. Peer reviewer responsiblities are found in the GSI wiki and cut-n-pasted below

At least two reviewers must be assigned to each PR. As a courtesy please ask a potential reviewer if (s)he is available to review your issue before assigning them. If you are unable to assign peer reviewers to your PR, please pass the github usernames of your peer reviewers to the Handling Reviewer for your PR. Please note that those on the Handling Review team can not serve as peer reviewers.

The reviewer’s job is to look at the changes that were made to the branch to ensure the code does what is expected and that the coding standards are adhered to. The reviewer should also ensure that all required tests have been done and results documented. If the reviewer has any questions or requires further tests these comments are made in the review. The developer responds. The reviewer examines updates in response to review comments. This interaction continues until reviewer and develop agree upon the changes. All merge requests require the approval of at least two reviewers. If changes are committed to the branch as a result of the review process, the regression tests should be rerun.

XuanliLi-NOAA · 2024-03-25T15:06:30Z

@RussTreadon-NOAA , thanks for the clarification.

RussTreadon-NOAA · 2024-03-25T15:12:26Z

@ADCollard , thank you for running ctests.

Yes, global_4denvar will, at present, fail for three reasons.

develop gsi.x aborts if we run global_4denvar with dsfcalc=1 for atms_n21. This PR fixes this.
develop gsi.x runs to completion with dsfcalc=0 for atms_n21 but generates different results due to calc_fov_crosstrk.
ADCollard:dsfcalc_fix is one commit behind develop. PR #722 fixed a read_nsstbufr.f90 bug which alters the number of assimilated sst observations.

We need to merge the latest develop into ADCollard:dsfcalc_fix to remove 3. We can work through 1 by using a different namelist in the contrl global_4denvar. 2 is the actual difference associated with the PR.

RussTreadon-NOAA · 2024-03-25T15:27:11Z

@ADCollard, if it is helpful I have Orion and Hercules ctests results. All tests pass except global_4denvar for the reasons noted above. I can post the results so you can focus on Cactus and Hera. Does this work for you?

RussTreadon-NOAA · 2024-03-25T16:57:24Z

Orion and Hercules ctests

Update ADCollard:dsfcalc_fix with develop. Build and run ctests on both MSU machines with the following results:

Orion

Test project /work2/noaa/da/rtreadon/git/gsi/pr727/build
    Start 1: global_4denvar
    Start 5: hafs_4denvar_glbens
    Start 6: hafs_3denvar_hybens
    Start 2: rtma
    Start 4: netcdf_fv3_regional
    Start 3: rrfs_3denvar_glbens
    Start 7: global_enkf
1/7 Test #4: netcdf_fv3_regional ..............   Passed  2047.69 sec
2/7 Test #3: rrfs_3denvar_glbens ..............   Passed  2050.56 sec
3/7 Test #7: global_enkf ......................   Passed  2239.34 sec
4/7 Test #2: rtma .............................   Passed  2591.42 sec
5/7 Test #6: hafs_3denvar_hybens ..............   Passed  2726.43 sec
6/7 Test #5: hafs_4denvar_glbens ..............   Passed  2901.67 sec
7/7 Test #1: global_4denvar ...................***Failed  3486.01 sec

86% tests passed, 1 tests failed out of 7

Total Test time (real) = 3486.03 sec

The following tests FAILED:
          1 - global_4denvar (Failed)

The global_4denvar failure is expected. global_4denvar.sh used in the test was modified to use a different namelist for updat and contrl. dsfcalc=1 in updat jobs. dsfcalc=0 in contrl jobs. The initial atms_n21 fit statistics differ. Below are summary statistics for atms_n21 prior to the first outer loop

< o-g 01 rad  n21       atms             623458         3388         1786    1941.5       1941.5       1.0871       1.0
871
---
> o-g 01 rad  n21       atms             623458         3388         1828    1949.0       1949.0       1.0662       1.0
662

< is contrl. > is updat. We see an increase in the number of assimilated observations with the changes in this PR. global_4denvar runs on Orion and Hercules show the same difference.

Hercules

Test project /work/noaa/da/rtreadon/git/gsi/pr727/build
    Start 1: global_4denvar
    Start 3: rrfs_3denvar_glbens
    Start 5: hafs_4denvar_glbens
    Start 6: hafs_3denvar_hybens
    Start 2: rtma
    Start 7: global_enkf
    Start 4: netcdf_fv3_regional
1/7 Test #4: netcdf_fv3_regional ..............   Passed  485.21 sec
2/7 Test #3: rrfs_3denvar_glbens ..............   Passed  488.52 sec
3/7 Test #7: global_enkf ......................   Passed  733.73 sec
4/7 Test #2: rtma .............................   Passed  967.58 sec
5/7 Test #6: hafs_3denvar_hybens ..............   Passed  1097.55 sec
6/7 Test #5: hafs_4denvar_glbens ..............   Passed  1103.01 sec
7/7 Test #1: global_4denvar ...................***Failed  1682.73 sec

86% tests passed, 1 tests failed out of 7

Total Test time (real) = 1682.75 sec

RussTreadon-NOAA · 2024-03-26T12:27:08Z

@xincjin-NOAA and @XuanliLi-NOAA : I will add both of you to the PR as peer reviewers. If one or both of you have time to review this PR, your doing so would be very much appreciated.

@ADCollard , before we merge this PR into develop, would you please set dsfcalc=1 for atms_n21 on line 184 of regression/regression_namelists.sh. Thank you.

XuanliLi-NOAA

Changes look good to me.

xincjin-NOAA

The changes are good.

ADCollard · 2024-03-26T14:55:01Z

Ctests run on WCOSS with dfscalc=0:

Test project /lfs/h2/emc/da/noscrub/andrew.collard/git/GSI_ADC_dsfcalc/build
    Start 1: global_4denvar
1/7 Test #1: global_4denvar ...................   Passed  1683.00 sec
    Start 2: rtma
2/7 Test #2: rtma .............................***Failed  971.80 sec
    Start 3: rrfs_3denvar_glbens
3/7 Test #3: rrfs_3denvar_glbens ..............   Passed  486.24 sec
    Start 4: netcdf_fv3_regional
4/7 Test #4: netcdf_fv3_regional ..............   Passed  485.31 sec
    Start 5: hafs_4denvar_glbens
5/7 Test #5: hafs_4denvar_glbens ..............   Passed  1219.72 sec
    Start 6: hafs_3denvar_hybens
6/7 Test #6: hafs_3denvar_hybens ..............   Passed  1213.46 sec
    Start 7: global_enkf
7/7 Test #7: global_enkf ......................***Failed  850.69 sec

71% tests passed, 2 tests failed out of 7

Total Test time (real) = 6910.65 sec

The following tests FAILED:
          2 - rtma (Failed)
          7 - global_enkf (Failed)

Both failures are walltime. I assume this is not an issue.

+ echo 'The runtime for rtma_hiproc_updat is 193.774327 seconds.  This has exceeded maximum allowable threshold time of 181.380489 seconds,'
 'The runtime for global_enkf_hiproc_updat is 128.343527 seconds.  This has exceeded maximum allowable threshold time of 128.037171 seconds,'

There are some weird warnings about ncdump not being found:

/lfs/h2/emc/da/noscrub/andrew.collard/git/GSI_ADC_dsfcalc/regression/regression_test.sh: line 316: ncdump: command not found

I am not sure why this would be as the netcdf module should be loaded.

ADCollard · 2024-03-26T15:08:58Z

Hera ctest results:

Test project /scratch1/NCEPDEV/da/Andrew.Collard/git/GSI_ADC_dsfcalc/build
    Start 1: global_4denvar
1/7 Test #1: global_4denvar ...................   Passed  1807.94 sec
    Start 2: rtma
2/7 Test #2: rtma .............................   Passed  972.46 sec
    Start 3: rrfs_3denvar_glbens
3/7 Test #3: rrfs_3denvar_glbens ..............***Failed  609.88 sec
    Start 4: netcdf_fv3_regional
4/7 Test #4: netcdf_fv3_regional ..............   Passed  487.20 sec
    Start 5: hafs_4denvar_glbens
5/7 Test #5: hafs_4denvar_glbens ..............   Passed  1344.82 sec
    Start 6: hafs_3denvar_hybens
6/7 Test #6: hafs_3denvar_hybens ..............   Passed  1103.60 sec
    Start 7: global_enkf
7/7 Test #7: global_enkf ......................   Passed  754.95 sec

86% tests passed, 1 tests failed out of 7

Total Test time (real) = 7080.93 sec

The following tests FAILED:
	  3 - rrfs_3denvar_glbens (Failed)

Again, it is a runtime failure:

+ echo 'The runtime for rrfs_3denvar_glbens_loproc_updat is 187.609595 seconds.  This has exceeded maximum allowable threshold time of 101.867415 seconds,'

ADCollard · 2024-03-26T15:09:34Z

Re-running ctests with dsfcalc=1 for NOAA-21 ATMS.

ADCollard · 2024-03-26T15:10:53Z

I am seeing the same ncdump warning on Hera, e.g.:

/scratch1/NCEPDEV/da/Andrew.Collard/git/GSI_ADC_dsfcalc/regression/regression_test_enkf.sh: line 269: ncdump: command not found

Investigating....

RussTreadon-NOAA

Thank you @ADCollard for updating regression/regression_namelists.sh.

Approve.

ADCollard · 2024-03-26T18:14:00Z

Hera tests with dsfcalc=1:

Test project /scratch1/NCEPDEV/da/Andrew.Collard/git/GSI_ADC_dsfcalc/build
    Start 1: global_4denvar
    Start 2: rtma
    Start 3: rrfs_3denvar_glbens
    Start 4: netcdf_fv3_regional
    Start 5: hafs_4denvar_glbens
    Start 6: hafs_3denvar_hybens
    Start 7: global_enkf
1/7 Test #1: global_4denvar ...................***Failed  1684.39 sec
2/7 Test #4: netcdf_fv3_regional ..............   Passed  1688.18 sec
3/7 Test #3: rrfs_3denvar_glbens ..............   Passed  1691.28 sec
4/7 Test #7: global_enkf ......................   Passed  2015.96 sec
5/7 Test #2: rtma .............................   Passed  2170.57 sec
6/7 Test #6: hafs_3denvar_hybens ..............   Passed  2364.02 sec
7/7 Test #5: hafs_4denvar_glbens ..............   Passed  2424.20 sec

86% tests passed, 1 tests failed out of 7

Total Test time (real) = 2424.23 sec

The following tests FAILED:
	  1 - global_4denvar (Failed)

The global_4denvar failure is expected as results have changed.

ADCollard · 2024-03-26T18:15:56Z

Similar results for wcoss with dsfcalc=1:

Test project /lfs/h2/emc/da/noscrub/andrew.collard/git/GSI_ADC_dsfcalc/build
    Start 2: rtma
    Start 7: global_enkf
    Start 1: global_4denvar
    Start 5: hafs_4denvar_glbens
    Start 6: hafs_3denvar_hybens
    Start 3: rrfs_3denvar_glbens
    Start 4: netcdf_fv3_regional
1/7 Test #4: netcdf_fv3_regional ..............   Passed  484.94 sec
2/7 Test #3: rrfs_3denvar_glbens ..............   Passed  607.79 sec
3/7 Test #7: global_enkf ......................   Passed  916.54 sec
4/7 Test #1: global_4denvar ...................***Failed  1081.90 sec
5/7 Test #6: hafs_3denvar_hybens ..............   Passed  1212.11 sec
6/7 Test #2: rtma .............................   Passed  1269.48 sec
7/7 Test #5: hafs_4denvar_glbens ..............   Passed  1333.36 sec

86% tests passed, 1 tests failed out of 7

Total Test time (real) = 1333.52 sec

The following tests FAILED:
          1 - global_4denvar (Failed)

RussTreadon-NOAA · 2024-03-26T18:44:08Z

@ADCollard , any more tests you would like to run or changes to make to the PR?

If not, we have sufficient peer reviews with approvals along with ctests. This PR can be sent to the Handling Review Team for final approval.

ADCollard · 2024-03-26T18:47:15Z

@RussTreadon-NOAA I think this is good to go. Thanks!

ADCollard and others added 7 commits November 9, 2023 22:47

Fix cool layer output

57e45c4

Merge branch 'NOAA-EMC:develop' into develop

12a67af

Remove HIRS from regression tests

336d6fb

Merge branch 'NOAA-EMC:develop' into develop

a59f8ff

Merge branch 'NOAA-EMC:develop' into develop

0026f1e

Add NOAA-21,22,23 heights for FOV calculation

b3b83e5

Add spaces for consistency

9760539

RussTreadon-NOAA previously approved these changes Mar 24, 2024

View reviewed changes

RussTreadon-NOAA assigned ADCollard Mar 24, 2024

RussTreadon-NOAA added this to the GFS v17 GSI/EnKF development milestone Mar 25, 2024

RussTreadon-NOAA self-assigned this Mar 25, 2024

emilyhcliu approved these changes Mar 25, 2024

View reviewed changes

Merge branch 'NOAA-EMC:develop' into dsfcalc_fix

d399484

RussTreadon-NOAA requested review from xincjin-NOAA and XuanliLi-NOAA March 26, 2024 12:27

XuanliLi-NOAA approved these changes Mar 26, 2024

View reviewed changes

xincjin-NOAA approved these changes Mar 26, 2024

View reviewed changes

Set dsfcalc=1 for NOAA-21 ATMS

dfdc60e

ADCollard dismissed RussTreadon-NOAA’s stale review via dfdc60e March 26, 2024 15:03

RussTreadon-NOAA self-requested a review March 26, 2024 15:54

RussTreadon-NOAA approved these changes Mar 26, 2024

View reviewed changes

RussTreadon-NOAA merged commit 6d9ebbb into NOAA-EMC:develop Mar 26, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dsfcalc fix #727

Dsfcalc fix #727

ADCollard commented Mar 22, 2024 •

edited by RussTreadon-NOAA

Loading

RussTreadon-NOAA left a comment

RussTreadon-NOAA commented Mar 24, 2024

RussTreadon-NOAA commented Mar 24, 2024

xincjin-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

XuanliLi-NOAA commented Mar 25, 2024

ADCollard commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

XuanliLi-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 26, 2024

XuanliLi-NOAA left a comment

xincjin-NOAA left a comment

ADCollard commented Mar 26, 2024

ADCollard commented Mar 26, 2024

ADCollard commented Mar 26, 2024

ADCollard commented Mar 26, 2024

RussTreadon-NOAA left a comment

ADCollard commented Mar 26, 2024

ADCollard commented Mar 26, 2024

RussTreadon-NOAA commented Mar 26, 2024

ADCollard commented Mar 26, 2024

Dsfcalc fix #727

Dsfcalc fix #727

Conversation

ADCollard commented Mar 22, 2024 • edited by RussTreadon-NOAA Loading

RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

RussTreadon-NOAA commented Mar 24, 2024

RussTreadon-NOAA commented Mar 24, 2024

xincjin-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

XuanliLi-NOAA commented Mar 25, 2024

ADCollard commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

XuanliLi-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 25, 2024

RussTreadon-NOAA commented Mar 26, 2024

XuanliLi-NOAA left a comment

Choose a reason for hiding this comment

xincjin-NOAA left a comment

Choose a reason for hiding this comment

ADCollard commented Mar 26, 2024

ADCollard commented Mar 26, 2024

ADCollard commented Mar 26, 2024

ADCollard commented Mar 26, 2024

RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

ADCollard commented Mar 26, 2024

ADCollard commented Mar 26, 2024

RussTreadon-NOAA commented Mar 26, 2024

ADCollard commented Mar 26, 2024

ADCollard commented Mar 22, 2024 •

edited by RussTreadon-NOAA

Loading