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.

Sign up for GitHub

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

Jump to bottom

a quick fix for Issue:A indexing out of bounds issue shown in the global_4denvar regr… #681

Merged
merged 6 commits into from
Feb 8, 2024
Merged

a quick fix for Issue:A indexing out of bounds issue shown in the global_4denvar regr… #681

merged 6 commits into from
Feb 8, 2024

Conversation

TingLei-daprediction
Copy link
Contributor

@TingLei-daprediction TingLei-daprediction commented Jan 11, 2024 •
edited by RussTreadon-NOAA
Loading

In response to Issue:" A indexing out of bounds issue shown in the global_4denvar regression test in setuprad.f90 #676" (#676) and following both online and off-line discussions, it is decided to retain the abi2km part in setuprad.f90 ( for potential future development/improvement), while a simple index-related checks are added to prevent the indexing error when GSI is built with debug mode.
It is important to note that this indexing error does not impact GSI results (when GSI is built with optimization options) since abi2km is currently hardwire consfigured to be not used.

Fixes #676

DUE DATE for merger of this PR into develop is 2/22/2024 (six weeks after PR creation).

@ShunLiu-NOAA ShunLiu-NOAA self-requested a review January 11, 2024 14:28
XuLu-NOAA
XuLu-NOAA reviewed Jan 11, 2024
View reviewed changes
Copy link
Contributor

@XuLu-NOAA XuLu-NOAA left a comment

Choose a reason for hiding this comment

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

Hi, @TingLei-daprediction , is there a convention that the variables stay on the left side of the numbers in equation/condition? E.g. size(abi2km_bc) .ge. 2 instead of 2 .le. size(abi2km_bc). Other than that I don't have questions about the fix itself.

@TingLei-daprediction
Copy link
Contributor Author

@XuLu-NOAA when I have those lines, kind of like I first have "if (i > size (abi2km_bc) ) then ", where I replaced i with 2 , 3 and so on. let me know if you prefer the form as "size(abi2km_bc) > 2" and so on.

@RussTreadon-NOAA
Copy link
Contributor

As @wx20jjung notes we need understand the reason for the abi2km code.

Logical abi2km is local to setuprad.f90 and is initialized to .false.

  abi        = obstype == 'abi'
  abi2km     = .false.

  ssmis=ssmis_las.or.ssmis_uas.or.ssmis_img.or.ssmis_env.or.ssmis

I do not see anywhere in src/gsi where abi2km is set to .true.. I don't even see a mechanism by which abi2km can be set to .true. apart from changing the hardcoded value in setuprad.f90.

Since abi2km is hardwired to .false., the code block in which abi2km_bc is used, namely,

           if (abi2km .and. regional) then                                                                                                                                  
              pred(:,i) = zero
              if (i>=2 .and. i<=4) then
                 if (tb_obs(i) > 190.0_r_kind .and. tb_obs(i) < 300.0_r_kind) then
                    pred(1,i)=1.0_r_kind
                    pred(2,i)=tb_obs(i)-abi2km_bc(i)
                    pred(3,i)=(tb_obs(i)-abi2km_bc(i))**2
                    pred(4,i)=(tb_obs(i)-abi2km_bc(i))**3
                 end if
              end if
           end if

will never be executed. Hence Jim's question. Is the abi2km code obsolete, orphaned, or in need of a rewrite?

If we are not ready to remove or rewrite code related to abi2km and abi2km_bc, we can restrict the initialization of abi2km_bc to when abi2km and regional are true. That is, modify the initialization of abi2km_bc to

        if (abi2km .and. regional) then
           abi2km_bc = zero
           abi2km_bc(2) = 233.5_r_kind
           abi2km_bc(3) = 241.7_r_kind
           abi2km_bc(4) = 250.5_r_kind
        endif

Since abi2km is .false., neither this block nor the block which uses abi2km_bc will be entered in regional or global gsi runs.

@TingLei-daprediction
Copy link
Contributor Author

@RussTreadon-NOAA . I can't answer the question if ab2km is obsolete or it will be updated/refreshed in the future. That is why I retain them and give the current fix just to avoid indexing errors which would cause GSI abort when GSI is built with debug mode.
Second, if we use the conditions (abi2km.and.regional) , this would also prevent the abortion as reported for the global regression test as reported in the corresponding issue and leave further sorting out /treatment to the future if abi2km would be used in the future. Do you prefer this fix?

@JingCheng-NOAA
Copy link
Contributor

This piece of code related to abi2km seems like someone's testing code to me. But in case it was designed for future use, I agree we can block this part for now. And I am more toward to @RussTreadon-NOAA 's solution to block this part at the initialization part. I have no more questions related to this fix.

@JingCheng-NOAA
Copy link
Contributor

@RussTreadon-NOAA . I can't answer the question if ab2km is obsolete or it will be updated/refreshed in the future. That is why I retain them and give the current fix just to avoid indexing errors which would cause GSI abort when GSI is built with debug mode. Second, if we use the conditions (abi2km.and.regional) , this would also prevent the abortion as reported for the global regression test as reported in the corresponding issue and leave further sorting out /treatment to the future if abi2km would be used in the future. Do you prefer this fix?

@TingLei-daprediction then how about remove the regional in the condition, only use abi2km?

@TingLei-daprediction
Copy link
Contributor Author

@JingCheng-NOAA I have no preference on this!

@XuLu-NOAA
Copy link
Contributor

Can we do both? The if condition will ensure the initialization is not used by default. But index check also resolves one potential issue if the abi2km is turned on in the future. Initialized or not, it is still a bug.

@RussTreadon-NOAA
Copy link
Contributor

My preference is to entirely remove the abi2km code because it won't be executed given how it is currently implemented in setuprad.f90.

GSI source code is version controlled. Removing abi2km code does not remove it from the repository. A developer interested in the abi2km code can locate the PR at which it was removed via a search of github issues or PRs and, given this, checkout the GSI hash prior to the commit at which the code was removed.

@TingLei-daprediction
Copy link
Contributor Author

@XuLu-NOAA and @JingCheng-NOAA So, since @RussTreadon-NOAA has the same opinion as @wx20jjung, one of the developers for this part, that this abc2km could be removed, I have removed abc2km blocks and please review the current version. Thanks.

ShunLiu-NOAA
ShunLiu-NOAA previously approved these changes Jan 12, 2024
View reviewed changes
@ShunLiu-NOAA
Copy link
Contributor

@JingCheng-NOAA please review the new changes from Ting.

RussTreadon-NOAA
RussTreadon-NOAA previously approved these changes Jan 12, 2024
View reviewed changes
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.

Thank you, @TingLei-daprediction , for removing unused code from setuprad.f90

Approve.

@JingCheng-NOAA
Copy link
Contributor

JingCheng-NOAA commented Jan 12, 2024 via email

@ShunLiu-NOAA
Copy link
Contributor

@TingLei-daprediction Could you please run regression test on HERA, WCOSS and ORION?

@TingLei-NOAA
Copy link
Contributor

@ShunLiu-NOAA An update on the regression tests: on hera, all passed, on orion, netcdf_fv3_regional failed for :

The memory for netcdf_fv3_regional_loproc_updat is 303768 KBs.  This has exceeded maximum allowable memory of 278040 KBs,
resulting in Failure memthresh of the regression test.

Considering this PR's property, it is safe to say this memory increase has nothing to with GSI itself and this regression test could be regarded as "pass" in my opinion. The regression test results on wcoss2 are to be reported when it is available to me again.

@TingLei-daprediction
Issue:A indexing out of bounds issue shown in the global_4denvar regr…
0b71af8 
…ession test in setuprad.f90 NOAA-EMC#676, remove abc2km blocks

And Issue: AMSR2 Retrieval Predictor Caculation Went Wrong NOAA-EMC#678, added
test to abort with error message.
XuLu-NOAA
XuLu-NOAA approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@XuLu-NOAA XuLu-NOAA left a comment

Choose a reason for hiding this comment

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

Thanks for adding these comments!

@TingLei-NOAA TingLei-NOAA mentioned this pull request Jan 19, 2024
Closed
@TingLei-daprediction
Copy link
Contributor Author

@RussTreadon-NOAA I had removed this change in question from this PR and will document our discussion in the issue 678 and let the decision be made/documented there. Thanks.

@RussTreadon-NOAA
Copy link
Contributor

Thank you @TingLei-daprediction

@TingLei-NOAA
Copy link
Contributor

An update: the regressions tests on WCOSS2 all passed!

ShunLiu-NOAA
ShunLiu-NOAA previously approved these changes Jan 25, 2024
View reviewed changes
@RussTreadon-NOAA
Copy link
Contributor

@ShunLiu-NOAA and @TingLei-daprediction

With the merger of PR #670 into develop the changes to setuprad.f90 in this PR conflict with the updated develop. Resolution of the conflict is straight forward. See Cactus /lfs/h2/emc/da/noscrub/russ.treadon/git/gsi/pr681 for a working copy of branch feature/gsi_abi2km_index with the conflict to setuprad.f90 resolved.

@TingLei-NOAA , if you like I can attempt to push my updated working copy of feature/gsi_abi2km_index to your fork. This may not work. I don't know. I won't try unless you approve.

@TingLei-NOAA
Copy link
Contributor

TingLei-NOAA commented Jan 26, 2024 •
edited
Loading

@RussTreadon-NOAA Thanks for taking care of this conflict. I had sent the invitation to you as the collaborator on my fork and hence, after you accept it, you could push your change to this PR.

@RussTreadon-NOAA
Copy link
Contributor

Conflict resolved @ 35b8aaf.

@TingLei-NOAA , peer reviewers need to re-approve this PR along with @ShunLiu-NOAA , the handling reviewer.

@TingLei-NOAA
Copy link
Contributor

@XuLu-NOAA @JingCheng-NOAA your another round of reviewing for the updated PR will be appreciated.

@ShunLiu-NOAA
Copy link
Contributor

@TingLei-daprediction Could you please re-run regression test?

@TingLei-NOAA
Copy link
Contributor

@ShunLiu-NOAA All tests passed the reproducibility tests on hera, orion and wcoss2.
On each machine, a few tests failed for either exceeding time or memory , which are not considered issues for this PR.
In conclusion, this PR passes all regression tests on the above 3 machines.

@ShunLiu-NOAA
Copy link
Contributor

@TingLei-NOAA Thank you for completing regression test on WCOSS2, HERA and ORION.

@ShunLiu-NOAA ShunLiu-NOAA merged commit 2915685 into NOAA-EMC:develop Feb 8, 2024
4 checks passed
@TingLei-daprediction TingLei-daprediction deleted the feature/gsi_abi2km_index branch February 22, 2024 21:53
@xincjin-NOAA xincjin-NOAA mentioned this pull request Mar 6, 2024
Merged
7 tasks
xincjin-NOAA pushed a commit to xincjin-NOAA/GSI that referenced this pull request Mar 6, 2024
Revert "a quick fix for Issue:A indexing out of bounds issue shown in…
224cb8d 
… the global_4denvar regr… (NOAA-EMC#681)"

This reverts commit 2915685.

for test 2915685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussTreadon-NOAA RussTreadon-NOAA RussTreadon-NOAA left review comments

@emilyhcliu emilyhcliu emilyhcliu left review comments

@TingLei-NOAA TingLei-NOAA TingLei-NOAA left review comments

@xincjin-NOAA xincjin-NOAA xincjin-NOAA left review comments

@ShunLiu-NOAA ShunLiu-NOAA ShunLiu-NOAA approved these changes

@XuLu-NOAA XuLu-NOAA XuLu-NOAA approved these changes

@JingCheng-NOAA JingCheng-NOAA JingCheng-NOAA approved these changes

Assignees

@ShunLiu-NOAA ShunLiu-NOAA

@TingLei-daprediction TingLei-daprediction

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A indexing out of bounds issue shown in the global_4denvar regression test in setuprad.f90
8 participants
@TingLei-daprediction @RussTreadon-NOAA @JingCheng-NOAA @XuLu-NOAA @ShunLiu-NOAA @TingLei-NOAA @emilyhcliu @xincjin-NOAA