Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring in HAFSv1 Related Changes #608

Conversation

JingCheng-NOAA
Copy link
Contributor

@JingCheng-NOAA JingCheng-NOAA commented Aug 10, 2023

Description
Bring in HAFSv1 related maxobs changes and the capability of assimilating GOES-R high-resolution AMVs.
Resolved #599

Type of change
Use "maxobs" as a condition to check whether the number of observations exceeds the limit, to avoid the out of bound/dimension issue in read_anowbufr.f90 read_dbz_nc.f90 read_gmi.f90 read_goesglm.f90 read_radar.f90 read_radar_wind_ascii.f90
This update also added the capability of assimilating the CIMSS enhanced GOES-R AMVs in a new "satwhr" bufr file.
Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?
This updates passed the HAFS related regression tests. All tests are performed on Orion.

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

DUE DATE for this PR is 9/21/2023. If this PR is not merged into develop by this date, the PR will be closed and returned to the developer.

jswhit2 and others added 30 commits December 7, 2018 15:47
Conflicts:
	src/enkf/Makefile
…I_fv3_reg_ensemble

get new updates in the repository
…0 for following dual resolution capabilities
@yonghuiweng
Copy link

The regression tests have been finished and all passed with no issue on catcus/WCOSS2.
Test project /lfs/h2/emc/hur/noscrub/yonghui.weng/test/hafs_gsi_1/build
Start 1: global_3dvar
Start 2: global_4dvar
Start 3: global_4denvar
Start 4: hwrf_nmm_d2
Start 5: hwrf_nmm_d3
Start 6: rtma
Start 7: rrfs_3denvar_glbens
Start 8: netcdf_fv3_regional
Start 9: global_enkf
1/9 Test #8: netcdf_fv3_regional .............. Passed 553.20 sec
2/9 Test #5: hwrf_nmm_d3 ...................... Passed 692.49 sec
3/9 Test #7: rrfs_3denvar_glbens .............. Passed 736.37 sec
4/9 Test #4: hwrf_nmm_d2 ...................... Passed 737.80 sec
5/9 Test #9: global_enkf ...................... Passed 1100.41 sec
6/9 Test #6: rtma ............................. Passed 1152.29 sec
7/9 Test #3: global_4denvar ................... Passed 1615.97 sec
8/9 Test #2: global_4dvar ..................... Passed 1684.74 sec
9/9 Test #1: global_3dvar ..................... Passed 1735.01 sec

100% tests passed, 0 tests failed out of 9

Total Test time (real) = 1735.06 sec

@JingCheng-NOAA
Copy link
Contributor Author

JingCheng-NOAA commented Sep 13, 2023 via email

@hu5970
Copy link
Collaborator

hu5970 commented Sep 14, 2023

Hi Jing, When I check "Files changed" option, your branch shows many differences that are not from your PR.
Also your branch shows:
[JingCheng-NOAA](https://github.com/JingCheng-NOAA) wants to merge 242 commits into [NOAA-EMC:develop](https://github.com/NOAA-EMC/GSI/tree/develop) from [hafs-community:feature/hafsv1_maxobs_goesr_meso_amvs](https://github.com/hafs-community/GSI/tree/feature/hafsv1_maxobs_goesr_meso_amvs)"

I am not sure how fix those issues. You branch needs to show the differences from develop that are only from you and show the history that are only related to this PR. May be you need to rebase your branch to current develop branch? Could you try it?

Thanks,
Ming

@JingCheng-NOAA
Copy link
Contributor Author

JingCheng-NOAA commented Sep 14, 2023 via email

JingCheng-NOAA and others added 2 commits September 15, 2023 14:00
* Drop off observations when the number of obs exceeds maxobs to avoid the out of bound/dimension issue in read_anowbufr.f90 read_dbz_nc.f90 read_gmi.f90 read_goesglm.f90 read_radar.f90 read_radar_wind_ascii.f90. (From @yonghuiweng)
* Add the capability of assimilating the CIMSS enhanced GOES-R AMVs in a new satwhr bufr file. (From @lbi2018 and @yonghuiweng)
@JingCheng-NOAA JingCheng-NOAA force-pushed the feature/hafsv1_maxobs_goesr_meso_amvs branch from c3cca77 to 2cabebd Compare September 15, 2023 16:32
@JingCheng-NOAA
Copy link
Contributor Author

JingCheng-NOAA commented Sep 15, 2023 via email

src/gsi/read_satwnd.f90 Outdated Show resolved Hide resolved
src/gsi/read_satwnd.f90 Outdated Show resolved Hide resolved
@hu5970
Copy link
Collaborator

hu5970 commented Sep 18, 2023

@JingCheng-NOAA This PR looks good except some minor cleanings needed. Please check Shun and my comments. After code clean and regression regression test cases on Hera and WCOSS2, we should move forward to merge the code.

@ShunLiu-NOAA
Copy link
Contributor

Yes, I will delete !write(6,) 'NC005099 readin' . However, for line 1224, if you mean the "!write(6,)'itype= ',itype", it seems like other "elseif" block above (e.g. line 1219, line 1214, etc) keeps this one, so to be consistent, we have this line here. But if you think it is not necessary, I will delete the line as well.

On Mon, Sep 18, 2023 at 11:29 AM ShunLiu-NOAA @.> wrote: @.* commented on this pull request. ------------------------------ In src/gsi/read_satwnd.f90 <#608 (comment)>: > @@ -537,6 +539,9 @@ subroutine read_satwnd(nread,ndata,nodata,infile,obstype,lunout,gstime,twind,sis itype=246 else if(trim(subset) == 'NC005031') then ! WV clear sky/deep layer itype=247 + else if(trim(subset) == 'NC005099') then + itype=241 + !write(6,) 'NC005099 readin' Could you delete line !write(6,) 'NC005099 readin' here? ------------------------------ In src/gsi/read_satwnd.f90 <#608 (comment)>: > @@ -1209,6 +1217,11 @@ subroutine read_satwnd(nread,ndata,nodata,infile,obstype,lunout,gstime,twind,sis c_station_id='WV'//stationid c_sprvstg='WV' !write(6,)'itype= ',itype + else if(trim(subset) == 'NC005099') then ! WV clear sky/deep layer + itype=241 + c_station_id='IR'//stationid + c_sprvstg='IR' + !write(6,)'itype= ',itype Is it necessary to keep line 1224 here? — Reply to this email directly, view it on GitHub <#608 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAHEWIKKYJ2SNUYHDIWVFZDX3BSGXANCNFSM6AAAAAA3L7BU5Y . You are receiving this because you were mentioned.Message ID: @.***>

@JingCheng-NOAA I think line 1224 is not necessary. This line can be removed. Please run regression tests again before merging these modifications to "develop," as Ming suggested.

@JingCheng-NOAA
Copy link
Contributor Author

JingCheng-NOAA commented Sep 18, 2023 via email

@JingCheng-NOAA
Copy link
Contributor Author

I've made requested changes and updated the branch.
Here are the regression tests results on Hera:
Test project /scratch1/NCEPDEV/hwrf/save/Jing.Cheng/GSIversions/GSIhafs/build
Start 1: global_3dvar
Start 2: global_4dvar
Start 3: global_4denvar
Start 4: hwrf_nmm_d2
Start 5: hwrf_nmm_d3
Start 6: rtma
Start 8: netcdf_fv3_regional
Start 9: global_enkf
Start 7: rrfs_3denvar_glbens
1/9 Test #7: rrfs_3denvar_glbens .............. Passed 667.97 sec
2/9 Test #5: hwrf_nmm_d3 ...................... Passed 674.46 sec
3/9 Test #8: netcdf_fv3_regional .............. Passed 724.08 sec
4/9 Test #4: hwrf_nmm_d2 ...................... Passed 727.50 sec
5/9 Test #9: global_enkf ...................... Passed 745.02 sec
6/9 Test #6: rtma ............................. Passed 1271.29 sec
7/9 Test #3: global_4denvar ................... Passed 1626.61 sec
8/9 Test #1: global_3dvar ..................... Passed 1925.41 sec
9/9 Test #2: global_4dvar ..................... Passed 1983.34 sec

100% tests passed, 0 tests failed out of 9

Total Test time (real) = 1983.36 sec

The Regression tests on Dogwoods are ongoing, should be ready later on.

@yonghuiweng
Copy link

I started over the regression tests on catcus/wcoss2, and passed all 9 tests.
Test project /lfs/h2/emc/hur/noscrub/yonghui.weng/test/hafs_gsi/build
Start 1: global_3dvar
Start 2: global_4dvar
Start 3: global_4denvar
Start 4: hwrf_nmm_d2
Start 5: hwrf_nmm_d3
Start 6: rtma
Start 7: rrfs_3denvar_glbens
Start 8: netcdf_fv3_regional
Start 9: global_enkf
1/9 Test #8: netcdf_fv3_regional .............. Passed 665.87 sec
2/9 Test #5: hwrf_nmm_d3 ...................... Passed 676.19 sec
3/9 Test #4: hwrf_nmm_d2 ...................... Passed 729.30 sec
4/9 Test #7: rrfs_3denvar_glbens .............. Passed 793.33 sec
5/9 Test #9: global_enkf ...................... Passed 1138.70 sec
6/9 Test #6: rtma ............................. Passed 1271.40 sec
7/9 Test #3: global_4denvar ................... Passed 1615.20 sec
8/9 Test #2: global_4dvar ..................... Passed 1685.56 sec
9/9 Test #1: global_3dvar ..................... Passed 1733.73 sec

100% tests passed, 0 tests failed out of 9

Total Test time (real) = 1733.74 sec

ShunLiu-NOAA
ShunLiu-NOAA previously approved these changes Sep 19, 2023
@RussTreadon-NOAA
Copy link
Contributor

If we are adding HAFSv1 changes to NOAA-EMC/GSI, now is the time to replace the hwrf regression tests a with hafs test. When will this be done?

@ShunLiu-NOAA
Copy link
Contributor

Jing is working on preparing the HAFS DA test case. I think her next PR is to add the new HAFS test cases to regression test and remove the old HWRF test cases.

@RussTreadon-NOAA
Copy link
Contributor

My comments aren't showstoppers but a response is appreciated. Several review comments have unresolved conversations. It's good practice to resolve conversations when the reviewer and developer come to agreement.

@RussTreadon-NOAA
Copy link
Contributor

Thank you @ShunLiu-NOAA . I see issue #600 has been opened for this task. Good to know that we will replace the hwrf tests with hafs tests.

@JingCheng-NOAA
Copy link
Contributor Author

Yes, we will bring in 4 regression tests for HAFS as described in issue #600. I will create pull request later on once Ming reviewed my datasets.

Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ShunLiu-NOAA ShunLiu-NOAA merged commit d84a18f into NOAA-EMC:develop Sep 20, 2023
4 checks passed
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.

Bring in HAFSv1 related maxobs changes and the capability of assimilating GOES-R high-resolution AMVs