GitHub Issue NOAA-EMC/GSI#243. AMVs changes to v16x

[ilianagenkova]

This branch brings:

• code to write out AMVQ mitigation flag into NetCDF diag file
• code to read Metop-B/C AMVs and Him-8 AMVs from new BUFR
• station_id bug fix for polar winds

[ilianagenkova]

Two /fix files need to be updated -
global_convinfo.txt and prepobs_errtable.global
The correct versions of these files are in branch: master_AMVs_fix

How to be sure that the correct prepobs_errtable.global is used?
See if this is in it:
260 OBSERVATION TYPE
0.11000E+04 0.10000E+10 0.10000E+10 0.38000E+01 0.10000E+10 0.10000E+10
0.10500E+04 0.10000E+10 0.10000E+10 0.38000E+01 0.10000E+10 0.10000E+10

How to be sure that the correct global_convinfo.txt is used?
See if this is in it:
uv 244 3 -1 3.0 0 0 0 2.5 20.0 1.4 2.5 0.005000 0 0. 0. 0 0. 0. 15 -8
uv 244 4 -1 3.0 0 0 0 2.5 20.0 1.4 2.5 0.005000 0 0. 0. 0 0. 0. 15 -8
uv 244 5 -1 3.0 0 0 0 2.5 20.0 1.4 2.5 0.005000 0 0. 0. 0 0. 0. 15 -8

[MichaelLueken]

@ilianagenkova While looking through the issues, I can't find an issue 109 that corresponds with AMV changes for v16x. It appears as though associated with this work is issue #243. Please update the issue number in the commit message in your fork's master by:

1. git clone --recursive git@github.com:ilianagenkova/GSI.git update
2. cd update
3. git commit --amend
4. Please replace 109 with 243:
   GitHub Issue NOAA-EMC/GSI#243. AMVs changes:add AMVQ mitigation flag, Metop-BC and Him-8 new BUFR, station_id bug fix
5. Please push back to your fork using:
   git push origin master --force

Also, please let me know who you would like to have review your changes.

[ilianagenkova]

@MichaelLueken-NOAA , Trying:

git clone --recursive git@github.com:ilianagenkova/GSI.git update

returns:
Cloning into 'update'...
Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

[ilianagenkova]

@MichaelLueken-NOAA , I managed to fix the issue number in the PR, thanks! Please assign @HaixiaLiu-NOAA as reeviewer.

[HaixiaLiu-NOAA]

@ilianagenkova Can you please provide your regression test summary here? Thank you

[ilianagenkova]

@MichaelLueken-NOAA or @HaixiaLiu-NOAA, could you point me to the instructions for regression tests, thanks!

[MichaelLueken]

@ilianagenkova The regression tests currently use CMake's ctest capabilities. The end of the README in the repository shows how to use ctest to start the regression tests. The quick way is to go into your build directory and use:

ctest -j19

This will submit all 19 regression test configurations. The output will be in your noscrub/regression directory. Do a grep for Fail and please note any failures either in this PR or in your issue. If there are failures, please make sure that they are expected (especially for reproducibility).

[HaixiaLiu-NOAA]

In the setupw.f90, the rdiagbufr array has dimension (nreal, nobs). The 1st dimension nreal=ioff0. The ioff0 is increased by 1 (changed from 25 to 26), but the subroutine contents_binary_diag_was not modified, which does not look right to me. At least the rdiagbufr(26,ii) is missing. Should it be assigned as AMVQ?

[ilianagenkova]

@HaixiaLiu-NOAA, I didn't update routine contents_binary_diag_ so that rdiagbuf array would include AMVQ.
Three reasons for that:
-we can only write in one conv diag file (either binary or NetCDF) and lately most people use the NetCDF conv diag file.
-AMVQ is not used for QC decisions, it's only kept as a "good to have" for diagnostics parameter
-AMVQ is not populated for GOES-16, it is only populated for GOES-17 in warm months and in the 12&18UTC cycles, and it will go away for GOES-18
Therefore I updated only the contents_netcdf_diag_ routine.

[HaixiaLiu-NOAA]

@ilianagenkova We can write to both binary and netcdf diag files at the same time. It is controlled at the script level and If you set both netcdf_diag and binary_diag to be true. However, that is unnecessary. If you do NOT want to add AMVQ to the rdiagbufr array, then there is no need to change ioff0 from 25 to 26.

[HaixiaLiu-NOAA]

@MichaelLueken-NOAA Would you please add Brett.Hoover@noaa.gov to the reviewer list. One is Brett can observe the process and eventually he will need to do PR in the future as well. Secondly, Brett can contribute as well. @ilianagenkova What do you think?

[ilianagenkova]

@HaixiaLiu-NOAA, there is really no need to add AMVQ to the binary diag.
@MichaelLueken-NOAA , please add @BrettHoover-NOAA as a reviewer/watcher.
Thanks!

[MichaelLueken]

@HaixiaLiu-NOAA @ilianagenkov

I have added @BrettHoover-NOAA as a reviewer.

[MichaelLueken]

@ilianagenkova, While running the debug tests, the two configurations that use NetCDF diagnostic files failed with the following message:

forrtl: severe (408): fort: (2): Subscript #1 of the array DATA has value 27 which is greater than the upper bound of 26

This is occurring on line 1808 in setupw.f90. Printing out nreal values, I'm seeing that the failure is occurring because nreal is 26. Looking at the changes you made for this PR, you changed nreal to be 27 in read_satwnd.f90. I suspect the issue that is happening is prepbufr uv data is hitting line 1808 in setupw and failing.

I was able to make the debug compilation work by setting nreal to 27 for uvobs in read_prepbufr. Is this the correct fix? It would seem like changing read_prepbufr to account for changes to read_satwnd is not the correct answer. A new variable to ensure that only satwnd data uses the diagnostic write seems to be the better method for correcting the failure.

[ilianagenkova]

@MichaelLueken-NOAA Running the regression test returns:
[hfe02:/scratch1/NCEPDEV/da/Iliana.Genkova/v16x_update/update/build|master]$ ctest -j19
Test project /scratch1/NCEPDEV/da/Iliana.Genkova/v16x_update/update/build
Start 1: global_T62
Start 2: global_T62_ozonly
Start 3: global_4dvar_T62
Start 4: global_4denvar_T126
Start 5: global_fv3_4denvar_T126
Start 6: global_fv3_4denvar_C192
Start 7: global_lanczos_T62
Start 8: arw_netcdf
Start 9: arw_binary
Start 10: nmm_binary
Start 11: nmm_netcdf
Start 12: nmmb_nems_4denvar
Start 13: hwrf_nmm_d2
Start 14: hwrf_nmm_d3
Start 15: rtma
Start 16: global_enkf_T62
Start 17: netcdf_fv3_regional
Start 18: global_C96_fv3aero
Start 19: global_C96_fv3aerorad
1/19 Test #11: nmm_netcdf .......................***Failed 1321.20 sec
2/19 Test #9: arw_binary .......................***Failed 1681.49 sec
3/19 Test #8: arw_netcdf .......................***Failed 1681.49 sec
4/19 Test #2: global_T62_ozonly ................ Passed 1683.24 sec
5/19 Test #17: netcdf_fv3_regional ..............***Failed 1741.56 sec
6/19 Test #16: global_enkf_T62 .................. Passed 1748.01 sec
7/19 Test #3: global_4dvar_T62 ................. Passed 2886.84 sec
8/19 Test #19: global_C96_fv3aerorad ............ Passed 4750.93 sec
9/19 Test #13: hwrf_nmm_d2 ......................***Failed 6304.98 sec
10/19 Test #14: hwrf_nmm_d3 ......................***Failed 6304.99 sec
11/19 Test #10: nmm_binary .......................***Failed 8046.17 sec
12/19 Test #4: global_4denvar_T126 .............. Passed 8109.79 sec
13/19 Test #12: nmmb_nems_4denvar ................***Failed 8226.23 sec
14/19 Test #6: global_fv3_4denvar_C192 ..........***Failed 8466.41 sec
15/19 Test #18: global_C96_fv3aero ............... Passed 10693.91 sec
16/19 Test #7: global_lanczos_T62 ............... Passed 11532.55 sec
17/19 Test #15: rtma ............................. Passed 13703.22 sec
18/19 Test #1: global_T62 ....................... Passed 14176.66 sec
19/19 Test #5: global_fv3_4denvar_T126 .......... Passed 14417.88 sec
53% tests passed, 9 tests failed out of 19
Total Test time (real) = 14417.94 sec

The following tests FAILED:
6 - global_fv3_4denvar_C192 (Failed)
8 - arw_netcdf (Failed)
9 - arw_binary (Failed)
10 - nmm_binary (Failed)
11 - nmm_netcdf (Failed)
12 - nmmb_nems_4denvar (Failed)
13 - hwrf_nmm_d2 (Failed)
14 - hwrf_nmm_d3 (Failed)
17 - netcdf_fv3_regional (Failed)

The *out files for the failed tests are:
/scratch1/NCEPDEV/da/Iliana.Genkova/v16x_update/update/regression

I added you to my AMVs meeting tomorrow, to go over the log files and discuss the debug run.
thanks

[ilianagenkova]

@MichaelLueken-NOAA Regarding line 1808 in setupw.f90, are you convinced there is an index issue? I ran an experiment with that setup just fine. Could it be a netcdf concatenation problem, @CoryMartin-NOAA ? I am printing extra variables in the netcdf diag file, while I am not changing the binary diag array.

[MichaelLueken]

@ilianagenkova I see that you added me to the AMV tag up this morning, but I'm on leave from 11 am - 1 pm, then out for the rest of the day, so I will be unable to attend. Looking at the regression tests that failed, they failed due to an out-of-memory issue in SLURM. I was able to correct several of these issues, but not all. On Hera, please try replacing your regression/regression_param.sh file with:

/scratch1/NCEPDEV/da/Michael.Lueken/regression_param.sh

Please note that there are still issues with the nmm(b) configurations. I would recommend running the regression tests on WCOSS. Having said that, I have run the regression tests on WCOSS and all passed successfully.

As for the debug tests, both global_C96_aerorad and global_fv3_4denvar_C192 failed the debug test due to out of bound subscripts. This needs to be addressed before I can merge this work to the authoritative repository. Again, the regression tests pass, but the debug tests that I run found this issue.

[ilianagenkova]

@MichaelLueken-NOAA, I updated read_prepbufr.f90 and setupw.f90 to resolve the issue with the debug test.
The reason we have to update nreal to 27 in both read_satwnd and read_prepbufr is that every prepbufr file contains some satwnd data, which is not used in GSI, but it's nevertheless there (an obsolete feature). That way, no matter whether cdata_all is populated , i.e. in read_satwnd or read_prepbufr (find "Winds" section), the cdata_all will have consistent dimensions, i.e. 27 x nobs.
I compiled my master just fine. I also compiled it with
build_type=${1:-'DEBUFR'}
in /ush/build_all_cmake.sh
Is this enough for you to go on? What else do you want me to do?

[MichaelLueken]

@ilianagenkova So long as the code compiled after your modification, that is fine. I can't move forward until either @HaixiaLiu-NOAA or @BrettHoover-NOAA approve the PR. Once that is complete, I will go over the changes more thoroughly and then submit the work to the review committee (so long as there are no outstanding issues).

[HaixiaLiu-NOAA]

@ilianagenkova why the cdata_all(24,iout) is missing? If the 24th element should be missing, would it be better to reduce the cdata_all array to be nreal-1 for the 1st dimension?

[ilianagenkova]

@HaixiaLiu-NOAA, setupw.f90 processes winds from multiple sources (AMVs, aircraft, sondes). AMVs don't have a parameter equivalent to "cdata_all(24,iout) = cat" . Only the winds that come to setupw.f90 from read_prepbufr.f90 have it. Regardless of whether winds are read in by read_prepbufr or read_satwnd, they need to fit in one cdata_all, therefore the dimensions need to match.

[ilianagenkova]

@HaixiaLiu-NOAA , thanks for the quick review.

[MichaelLueken]

@ilianagenkova I have completed my review and the modifications look good. I have run the regression tests (both standard and debug) and all passed successfully following your update. I will be able to submit your work to the review next Tuesday, after the current work out for review has been merged to the authoritative repository.

Before this work is sent to the review committee, however, please go into your fork's master and use the following commands:

1. git reset --soft HEAD~1
2. git commit --amend
3. Save and close the commit message without altering the commit message
4. git push origin master --force

When you pushed the correction to allow the debug executable to run, you added another commit message. Only a single commit message will be accepted. These steps will undo the last update, while maintaining the modifications made to the allow the code to run in debug, make an amended commit, then push the change back.

If you have any questions, please let me know.

[ilianagenkova]

@MichaelLueken-NOAA Done! Thanks for the clear instructions.

[MichaelLueken]

@ilianagenkova Following the update to the authoritative master this morning, these is a conflict in src/gsi/setupw.f90. To update your master with the authoritative repository and correct the conflict:

1. git remote add upstream https://github.com/NOAA-EMC/GSI.git
2. git remote update
3. git rebase upstream/master
4. rm -rf libsrc
5. git submodule update
6. vi src/gsi/setupw.f90
7. Correct the minor conflict under REVISION HISTORY in the doc bloc on lines 222 - 229. It should look like:

!   2021-07-25 Genkova  - write AMVQ in diagnostic files
!   2021-10-xx  pondeca/morris/zhao - added observation provider/subprovider
!                         information in diagonostic file, which is used
!                         in offline observation quality control program (AutoObsQC)
!                         for 3D-RTMA (if l_obsprvdiag is true).
!

8. git add src/gsi/setupw.f90
9. git rebase --continue
10. git push origin master --force

I should be able to get this work out to the review committee tomorrow once these steps have been completed. Please let me know if you have any questions.

[ilianagenkova]

Changes done as instructed!

[MichaelLueken]

The due date for the review committee has passed with no feedback, so I will now give final approval to these changes and merge them to the authoritative repository.