-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fix write_incr #559
Fix write_incr #559
Conversation
…FV3 DA (FV3LAMDA)
src/gsi/write_incr.f90
Outdated
@@ -202,7 +211,7 @@ subroutine write_fv3_inc_ (grd,sp_a,filename,mype_out,gfs_bundle,ibin) | |||
iqg = getindex(svars3d,'qg') | |||
|
|||
istatus=0 | |||
call gsi_bundlegetpointer(gfs_bundle,'q', sub_qanl, iret); istatus=istatus+iret | |||
call gsi_bundlegetpointer(svalinc(ibin2),'q', sub_qanl, iret); istatus=istatus+iret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch on this, no idea why this one is different from the rest... does this have any implcations for current GFSv16?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jderber-NOAA I am a little confused here. In the following around line 313 (in this program), there is
! compute q increment sub_q = sub_qanl(:,:,:) - ges_q1(:,:,:,ibin)
So, seems to me, the original code intentionally gets analysis q in full at the above (line 205) .
I am wondering why the original treatment is used . However, if the change of line 214 is used, the following line 313 should also be changed too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree good catch. I do not understand why this variable is handled differently. But the intent of this change is not to change the science, but to make the code work. The line is reverted to use gfs_bundle and the tests resubmitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All regression tests passed and test of code was fine. I believe this is ready for re-review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not test but changes look straightforward and correct to me
As per the 4/10/2023 email sent to the GSI subversion email list, PRs must be merged within 6 weeks of creation. PRs not merged within the 6 week window will be returned to the originating developer (see GSI Wiki). This PR was opened 4/13/2023. The due date for this PR is set to 5/25/2023, six weeks after 4/13/2023. |
All regression tests pass. Two peer reviews approve. I'll reach out to the Handling Review Team for final approval and merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve.
Thanks Cory and Ting! Up to Russ now! |
I seem to be a few minutes (or less) behind everyone this morning! |
@jderber-NOAA A following up question , the previous assignment between gsi bundle (like currently line 195) while without defining the operator overload by "use gsi_bundlemod, only: assignment(=)" added in this PR, would it cause any actual impacts? Namely, will this PR create different increment (correct) fields in the generated increment files? I am not sure if current regression tests compare the generated increment files. |
Ting, That is a good question. I don't know. For my test case, the code would not complete without these changes so I could not compare. Could you do a comparison? If there is a difference, I am certain that the new is the more correct one. |
@jderber-NOAA I am not sure if current regression tests for global gsi will create the inc files. |
Merger into |
An update: Thanks to @RussTreadon-NOAA 's information, I directly compared the increment files in the regression test : global_3dvar on hera: in /scratch1/NCEPDEV/stmp2/Ting.Lei/GSI/tmpreg_global_3dvar, I ran
|
Given @TingLei-NOAA confirmation of identical increments go ahead and merge this PR into |
Fixes #558