-
Notifications
You must be signed in to change notification settings - Fork 151
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 indexing issues with correlated error in V16.x #233
Comments
This work is being done in my fork KristenBathmann/GSI, in release/gfsda.v16.x |
Given this caution should this work be done in a branch off the current
master? gfsda.v16.x will be sync'd with the master. This sync would
bring in the necessary changes. This bypasses potential problems when we
merge into the master.
…On Fri, Oct 15, 2021 at 1:16 PM Kristen Bathmann ***@***.***> wrote:
This work is being done in my fork KristenBathmann/GSI, in
release/gfsda.v16.x
The util/Correlated_Obs directory in here is very out of date with the
master. Care will need to be taken when merging this code into the master.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#233 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGNN635XBSMCYO5RYONXGNTUHBOXBANCNFSM5GCIY3LQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
So just so I am clear what you are suggesting, @RussTreadon-NOAA . Should we
I will work on Step number 3 once the other two are complete. |
I have put the potential fix into my branch. It works for gdasanal, but I don't have a script to test gfsanal. This will also need to be tested after adding and removing channels from the satinfo, for both gdasanal and gfsanal. You can test by grepping "cond=" in the stdout. It should get smaller when removing channels, and larger when adding, and obviously there should be no warnings about correlated error getting turned off. For the updates to the util, I changed how schind is defined in readsatobs.f90, and created an array in here called indRf, which is written to the covariance file in cov_calc.f90. It shouldn't be too difficult to merge this into the master. |
My comments are limited only to the changes Kristen needs to make. I read
her caution "Care will need to be taken when merging this code into the
master" and wondered if Kristen directly commit her changes to the master.
She knows util/Correlated_Obs so she should be able to safely navigate the
caution she raised. This way we don't need to cross the master bridge
later down the road. The approach you outline is equally valid. I am not
familiar with the correlated error code. Hence my desire to have Kristen
do the heavy lifting and get her changes into the master.
…On Fri, Oct 15, 2021 at 2:32 PM Andrew Collard ***@***.***> wrote:
So just so I am clear what you are suggesting, Russ.
Should we
1. Merge the master into v16.x
2. Merge 16.x back into the master
3. Introduce the latest changes that Kristen has introduced today into
the master at this point?
I will work on Step #3 <#3> once
the other two are complete.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#233 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGNN632GGOUJYDXNIIIAJQ3UHBXRRANCNFSM5GCIY3LQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
My comments are obsolete. Kristen's changes are already in her forked copy of release/gfsda.v16.x as of 33e46e6. This branch has been cloned, built, and run on Venus. The global_gsi.x runs past the point of previous failure for the 2021101500 gfs case. stdout does not contain the Rcov(Evals) values Kristen mentions in her comment. Instead, stdout contains
This run is using Rcov files taken from the master. Is this the problem? |
Please point to these covariance files: These are updated with different indices. |
Test rerun with Rcov files from
The updated Rcov files need to be committed. |
I created a branch to reflect what util/Correlated_Obs should look like when release/gfsda.v16.x is merged into the master. It is called v16.x_corrutil I updated this fix directory with covariance files for IASI-A (I only updated the IASI-B and CrIS previously). These Rcov files do need to committed. |
Reran the 2021101500 gdas case using Kristen's forked copy of release/gfsda.v16.x as of 33e46e6. Use the updated Rcov files Kristen provided. Also run 2021101500 gdas using authoritative release/gfsda.v16.x at 23abb36 with updated Rcov files from Kristen. All other fix files came from Kristen's release/gfsda.v16.x. I thought indexing was only a problem in gfs runs. I expected initial J terms to be identical between the two global_gsi.x executables for the 2021101500 gdas case. This is not true. The initial J for radiances differ. The authoritative gfsda.v16.x has 6.2847632847397670E+05. |
Using my updated fork copy requires the updated Rcov files, and this should work for both gdas and gfs. Using the current authoritative release/gfsda.v16.x requires the old Rcov files, but will only work for gdas. |
So we should expect differences in both gfs and gdas runs when using your updated fork with update Rcov files. This is what I see. |
Yes, you should expect the differences that you observed. If you rerun the gdasanal with the authoritative release/gfsda.v16.x and the old Rcov files, the result should be identical to using my fork with the new covariance files. |
I ran a clean test for 2021101600 gfs and gdas. Two GSI versions were checked out and installed on Mars: For all runs For 2021101600 gfs, the NOAA-EMC For 2021101600 gdas, the NOAA-EMC and Kristen's NOAA-EMC
Kristen's fork
However, the initial step sizes differs in the last few digits
Could this reflect different ordering of information in the vectors processed by pcgsoi and routines called from pcgsoi? At the start of the second outer loop the cost and gradient norm differ, though the leading digits are identical NOAA-EMC
Kristen's fork
The final cost and grad numbers are comparable Kristen's fork
Taken as a whole is the behavior we observe expected and acceptable? |
@RussTreadon-NOAA, it is not immediately apparent to me why we would see a difference in the step size from this change (possibly related to different treatment of passive obs?). I'd be OK with proceeding, but I'm going to take a couple of hours digging into the code if that is OK? |
Thank you, @ADCollard for looking at the code. I agree. I did not expect a difference for the gdas case. My guess is that the treatment of passive obs alters the order of information in the vectors which pcgsoi and the routines it calls process. This change in order leads to small roundoff differences in minimization calculations. Everything may be OK but it would be nice to have confirmation that everything is OK. |
@RussTreadon-NOAA There's one difference not from Kristen. John added an extra line to genqsat.f90 that is not in Kristen's branch. And I'm guessing that might be more likely to produce the change we've seen. Should I just go ahead and add it and then you can rerun? |
Excellent find, @ADCollard. You solved the mystery! Details matter. I missed this critical detail. I added John's genqsat.f90 change to my working copy of Kristen's branch. global_gsi.x was rebuilt and the 2021101600 gdas case is being rerun. The job has not finished but it's into the minimization on the second outer loop. The minimization stats from the rerun are identical to the authoritative gfsda.v16.x run for all printed digits. We are merging Kristen's branch into the authoritative branch. John's change to genqsat.f90 is already in the authoritative branch. I think it is sufficient that you explained the reason for the observed differences. We don't need to commit the John's genqsat.f90 to Kristen's branch. We need to get Kristen's updated Rcov files into fix ( |
Just to confirm that with the
|
So I have added the 8 Rcov files to DA_GFSv16.x_global_only (land and sea files for the three IASIs and one each for the two CrISs). Thanks to @RussTreadon-NOAA and @CatherineThomas-NOAA for their patience as I remembered how to do it again! |
@ADCollard committed Rcov files with revised channel number information to Update NOAA-EMC/GSI release/gfsda.v16.x |
GSI Issue #233: Fix indexing issues in correlated error
A recent update to release/gfsda.v16.x allowed for flexibility in channel selection in the satinfo while preserving correlated error. Testing with GDAS was successful, but testing with GFS failed. This is because the gfsanal and gdasanal use different channel indices. The gfsanal completely ignores monitored channels and generates a satinfo file that does not contain them.
Each covariance file is generated as
open(26,file=trim(cov_file),form='unformatted')
write(26) nch_active, nctot, reclen
write(26) indR
write(26) Rcov
close(26)
nch_active is the number of active channels, for example 174 for IASI
nctot is the total number of channels, for example 616 for IASI
indR are satinfo relative indices, for example 1, 5, 9, 11... for IASI. These would correspond to channels 16, 38, 49 and 51.
Rcov is the covariance matrix.
The correlated obsmod in the master has logic in various routines to handle the inconsistency in channel indexing between the gfsanal and the gdasanal. The update to the set_routine in correlated_obsmod.F90 build a covariance matrix from the file based on the channels in the satinfo. Because of the way the channel indices are specified in the covariance file (1,5,9,11 rather than 16,38,49,51) there is no way for set_ to know which channel has been removed or added in the satinfo, and won't know which entries in Rcov to use. Fixing this issue will therefore require changing the indices in the covariance file (to 16,38,49,51,...), changing correlated_obsmod.F90 and updating the covariance utility util/Correlated_Obs to write the files correctly.
To make matters more complicated, GMAO does not have this indexing issue in their code, and the logical GMAO_ObsErrorCov will need to be used in set_.
The text was updated successfully, but these errors were encountered: