GitHub Issue NOAA-EMC/GSI#353. RO Updates

[ADCollard]

This pull request is related to Issue #353.

This updates the src/ components of the changes proposed by @LidiaCucurull-NOAA. Fix and workflow updates will follow.

One very minor change to src/gsi/pcgsoi.f90 is also included which is a change suggested by @jderber-NOAA to improve convergence.

Regression tests have passed except where differences are expected.

[MichaelLueken]

Hi Andrew,

I went ahead and completed a quick initial visual inspection of the changes. There were two minor issues, both in src/gsi/setupbend.f90:

1. Please ensure 132 character limit per line on lines 587 - 598.
2. Please add proper precision to real value 0.05 on line 798.

Also, I noted a second commit message. To remove the extraneous commit messages, please do the following in your local copy of RO_Updates:

1. git reset --soft HEAD~1 (this will remove the current commit message, but keep the changes to the four files).
2. git commit -m "GitHub issue NOAA-EMC/GSI#353: GNSS RO Upgrade" (this will add back in the correct commit message that you currently have).
3. git push origin RO_Updates --force (this will allow you to push the corrected commit message and other changes back to your fork and update this PR).

If you have any questions or encounter any issues, please let me know.

[MichaelLueken]

Please add proper precision to real value, 0.05 (i.e., 0.05_r_kind)

[MichaelLueken]

Please ensure that the 132 character limit per line is met. There are several empty spaces on lines 586, 587, 589, 591, 593, and line 598. After the removal of the blank spaces, line 587 will still exceed the 132 character limit per line. Please break up line 587 so that the 132 character limit is maintained.

[ADCollard]

I got the following error. What do you suggest?

git push origin RO_Updates

To https://github.com/ADCollard/GSI.git ! [rejected] RO_Updates -> RO_Updates (non-fast-forward) error: failed to push some refs to 'https://github.com/ADCollard/GSI.git' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Integrate the remote changes (e.g. hint: 'git pull ...') before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details.

…

On Mon, Apr 4, 2022 at 11:42 AM MichaelLueken-NOAA ***@***.***> wrote: ***@***.**** commented on this pull request. Hi Andrew, I went ahead and completed a quick initial visual inspection of the changes. There were two minor issues, both in src/gsi/setupbend.f90: 1. Please ensure 132 character limit per line on lines 587 - 598. 2. Please add proper precision to real value 0.05 on line 798. Also, I noted a second commit message. To remove the extraneous commit messages, please do the following in your local copy of RO_Updates: 1. git reset --soft HEAD~1 (this will remove the current commit message, but keep the changes to the four files). 2. git commit -m "GitHub issue NOAA-EMC/GSI#353: GNSS RO Upgrade" (this will add back in the correct commit message that you currently have). 3. git push origin RO_Updates --force (this will allow you to push the corrected commit message and other changes back to your fork and update this PR). If you have any questions or encounter any issues, please let me know. ------------------------------ In src/gsi/setupbend.f90 <#354 (comment)>: > ! Accumulate diagnostic information rdiagbuf( 5,i) = (data(igps,i)-dbend)/data(igps,i) ! incremental bending angle (x100 %) data(igps,i)=data(igps,i)-dbend !innovation vector + +! Remove very large simulated values + if(dbend > 0.05) then Please add proper precision to real value, 0.05 (i.e., 0.05_r_kind) ------------------------------ In src/gsi/setupbend.f90 <#354 (comment)>: > + if ((tpdpres(i)<ref_rad(top_layer_SR+1)).and.(tpdpres(i)<ref_rad(bot_layer_SR))) then !obs below model SR/close-to-SR layer + qcfail(i)=.true. + elseif (tpdpres(i)>ref_rad(top_layer_SR+5)) then ! obs above SR/close-to-SR layer + qcfail(i)=.false. + if(hob < top_layer_SR+1) then !location might be aliased to the lower section of the non-monotonicity + hob = tpdpres(i) + call grdcrd1(hob,ref_rad(top_layer_SR+1),nsig-top_layer_SR-1,1) ! only non-monotonic section above SR layer + data(ihgt,i) = hob+top_layer_SR + hob = hob+top_layer_SR + rdiagbuf(19,i) = hob + endif + else ! obs inside model SR/shadow or close-to-SR layer Please ensure that the 132 character limit per line is met. There are several empty spaces on lines 586, 587, 589, 591, 593, and line 598. After the removal of the blank spaces, line 587 will still exceed the 132 character limit per line. Please break up line 587 so that the 132 character limit is maintained. — Reply to this email directly, view it on GitHub <#354 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJTUMJAL7PTWLYI5HHXBB2TVDMEWBANCNFSM5SPZJSXA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ADCollard]

... I meant to ask, should I do the git pull and then git push or would I need to do something more? ADC On Mon, Apr 4, 2022 at 12:51 PM Andrew Collard - NOAA Federal < ***@***.***> wrote:

…

I got the following error. What do you suggest? > git push origin RO_Updates To https://github.com/ADCollard/GSI.git ! [rejected] RO_Updates -> RO_Updates (non-fast-forward) error: failed to push some refs to 'https://github.com/ADCollard/GSI.git' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Integrate the remote changes (e.g. hint: 'git pull ...') before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details. On Mon, Apr 4, 2022 at 11:42 AM MichaelLueken-NOAA < ***@***.***> wrote: > ***@***.**** commented on this pull request. > > Hi Andrew, > > I went ahead and completed a quick initial visual inspection of the > changes. There were two minor issues, both in src/gsi/setupbend.f90: > > 1. Please ensure 132 character limit per line on lines 587 - 598. > 2. Please add proper precision to real value 0.05 on line 798. > > Also, I noted a second commit message. To remove the extraneous commit > messages, please do the following in your local copy of RO_Updates: > > 1. git reset --soft HEAD~1 (this will remove the current commit > message, but keep the changes to the four files). > 2. git commit -m "GitHub issue NOAA-EMC/GSI#353: GNSS RO Upgrade" > (this will add back in the correct commit message that you currently have). > 3. git push origin RO_Updates --force (this will allow you to push > the corrected commit message and other changes back to your fork and update > this PR). > > If you have any questions or encounter any issues, please let me know. > ------------------------------ > > In src/gsi/setupbend.f90 > <#354 (comment)>: > > > > ! Accumulate diagnostic information > rdiagbuf( 5,i) = (data(igps,i)-dbend)/data(igps,i) ! incremental bending angle (x100 %) > > data(igps,i)=data(igps,i)-dbend !innovation vector > + > +! Remove very large simulated values > + if(dbend > 0.05) then > > Please add proper precision to real value, 0.05 (i.e., 0.05_r_kind) > ------------------------------ > > In src/gsi/setupbend.f90 > <#354 (comment)>: > > > + if ((tpdpres(i)<ref_rad(top_layer_SR+1)).and.(tpdpres(i)<ref_rad(bot_layer_SR))) then !obs below model SR/close-to-SR layer > + qcfail(i)=.true. > + elseif (tpdpres(i)>ref_rad(top_layer_SR+5)) then ! obs above SR/close-to-SR layer > + qcfail(i)=.false. > + if(hob < top_layer_SR+1) then !location might be aliased to the lower section of the non-monotonicity > + hob = tpdpres(i) > + call grdcrd1(hob,ref_rad(top_layer_SR+1),nsig-top_layer_SR-1,1) ! only non-monotonic section above SR layer > + data(ihgt,i) = hob+top_layer_SR > + hob = hob+top_layer_SR > + rdiagbuf(19,i) = hob > + endif > + else ! obs inside model SR/shadow or close-to-SR layer > > Please ensure that the 132 character limit per line is met. There are > several empty spaces on lines 586, 587, 589, 591, 593, and line 598. After > the removal of the blank spaces, line 587 will still exceed the 132 > character limit per line. Please break up line 587 so that the 132 > character limit is maintained. > > — > Reply to this email directly, view it on GitHub > <#354 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AJTUMJAL7PTWLYI5HHXBB2TVDMEWBANCNFSM5SPZJSXA> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[emilyhcliu]

@ADCollard I got the same error before. I just did what was suggested by the system (do "git pull" before doing "git push")

[MichaelLueken]

@ADCollard When you use the git reset --soft HEAD~1 command, you will need to use:

git push origin RO_Updates --force in order for GitHub to accept the update.

Using git pull, then git push origin RO_Updates will bring back the extra commit message, which isn't desired.

[ADCollard]

Thanks, Mike. I think I have it right now. There are two commit messages, but the second one addressed your requested changes. ADC

…

On Mon, Apr 4, 2022 at 1:00 PM MichaelLueken-NOAA ***@***.***> wrote: @ADCollard <https://github.com/ADCollard> When you use the git reset --soft HEAD~1 command, you will need to use: git push origin RO_Updates --force in order for GitHub to accept the update. Using git pull, then git push origin RO_Updates will bring back the extra commit message, which isn't desired. — Reply to this email directly, view it on GitHub <#354 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJTUMJBVSXEOTSSIMX4QGDTVDMN35ANCNFSM5SPZJSXA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[MichaelLueken]

@ADCollard Thank you very much for removing the previous commit message and making the requested updates to src/gsi/setupbend.f90. To return the PR to having a single commit message:

1. git reset --soft HEAD~1
2. git commit --amend
3. Please save and close commit message window without altering commit message
4. git push origin RO_Updates --force

Then, so long as there are no further feedback from reviewers (and if there is additional changes to be made, please use git commit --amend rather than git commit after the changes have been made), I should be able to get this work out to the review committee shortly.

Please let me know if you have any questions or encounter issues.

[HaixiaLiu-NOAA]

The sentinel-6 needs to added into the global_convinfo.txt. And one minor comment on the code changes: Is there a need that the Jacobian QC is changed as a option to switch on/off instead of completely being removed in case we need it in the future?

[ADCollard]

@HaixiaLiu-NOAA , I was going to add the fix changes later as suggested by @MichaelLueken-NOAA. My understanding is that the Jacobian QC is no longer needed because of other QC modifications (p7 of the attached presentation), so making this switchable is probably not necessary.

[CatherineThomas-NOAA]

Has @dtkleist signed off on removing the Jacobian QC? When the bug fixes were added in DO-3, he pushed keeping the QC as a risk mitigation measure. It doesn't do any harm if it sticks around and doesn't catch anything, correct? Or would this QC stop good observations from getting in now that the bugs and other QC have been addressed?

[LidiaCucurull-NOAA]

The Jacobian QC is no longer necessary with the recent changes and bug fixes - which were not available when DO-3 was implemented. Keeping it would stop good observations from getting into the system. Lidia

…

-- Lidia Cucurull, PhD Chief Scientist & Deputy Director, NOAA Quantitative Observing System Assessment Program (QOSAP) NOAA OAR Principal Investigator for GNSS-RO Atlantic Oceanographic and Meteorological Laboratory (AOML) 720-771-0830 (M)

On Apr 5, 2022, at 8:50 AM, CatherineThomas-NOAA ***@***.***> wrote: Has @dtkleist <https://github.com/dtkleist> signed off on removing the Jacobian QC? When the bug fixes were added in DO-3, he pushed keeping the QC as a risk mitigation measure. It doesn't do any harm if it sticks around and doesn't catch anything, correct? Or would this QC stop good observations from getting in now that the bugs and other QC have been addressed? — Reply to this email directly, view it on GitHub <#354 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFT3BH6DU6AUABORSYM3FLVDRHK7ANCNFSM5SPZJSXA>. You are receiving this because you were mentioned.

[ADCollard]

Based on feedback from @HaixiaLiu-NOAA and @CatherineThomas-NOAA and an abundance of caution, I have modified the code to allow the Jacobian QC to be switchable with the addition of the gps_jacqc variable in the OBSQC namelist. This is set to .false. by default, so the results do not change from @LidiaCucurull-NOAA 's original unless this variable is set.

[HaixiaLiu-NOAA]

changes look good to me. Approved.

[CatherineThomas-NOAA]

Changes look good.

[ADCollard]

I was planning to add a change to the exglobal_atmos_analysis.sh, but I think this is best done in the workflow. So I believe this is good to go except for the fix changes.

@HaixiaLiu-NOAA, I think you need to re-approve as I made the jacobian QC change after you approved.

[HaixiaLiu-NOAA]

changes are good.

[MichaelLueken]

There has been no feedback from the review committee for this PR, so I will now give final approval and merge this work to the authoritative repository.

[ADCollard]

Thanks @MichaelLueken-NOAA . I do still have fix file changes - should these be coordinated with one of the other pull requests?

[MichaelLueken]

Hi @ADCollard. Fix files can go in so long as there are no changes out to the review committee. Please follow the guidance to update the fix files, then give me the name of the branch in the fix submodule so I can merge it in. All fix branches need to be manually merged back into rev2 by me, so this is the best and easiest way to work fix changes.