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.

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

A quick fix for incorrect usage of real(i_kind) in mg_input.f90 #760

Conversation

TingLei-daprediction
Copy link
Contributor

Description
A quick fix for incorrect usage of real(i_kind) in mg_input.f90 , which was identified by D. Kokron.
Fixes #757
Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?
The building succeeded. And the changed code mg_input.f90 is not used by other codes in the current GSI and hence the change hadn't and would not affect any GSI runs.

Coauthor : D. Kokron

…ich was identified by D. Kokron. NOAA-EMC#757 .    Co-author : D. Kokron
@RussTreadon-NOAA
Copy link
Contributor

@TingLei-NOAA, @hu5970 , and @ShunLiu-NOAA: I did not realize that we do not exercise mgbf in any of the regional ctests. This is not good. We intend to use mgbf in an operational realizations of the GSI, right? If true, we should exercise mgbf code as part of our standard GSI regression (ctest) suite.

What is the impact of correctly defining the variables in question as integer(i_kind) instead of real(i_kind)? Someone needs to run a test to quantify the impact, if any, of this bug fix.

@ShunLiu-NOAA
Copy link
Contributor

@RussTreadon-NOAA, we have no plan to use MGBF in RRFSv1. We expect to use it in RRFSv2 with JEDI.

@RussTreadon-NOAA
Copy link
Contributor

Thank you @ShunLiu-NOAA for this very surprising news. If we do not intend to use mbgf in an operational realization of GSI why did we add it to the GSI repository?

@ShunLiu-NOAA
Copy link
Contributor

I am not sure if 3DRTMA will implement this. Also, for JEDI-MGBF development, it is necessary to test MGBF with GSI first. This is what Ting and other developers are working on.

@RussTreadon-NOAA
Copy link
Contributor

Thanks, @ShunLiu-NOAA. Given your reply we need a mgbf ctest in GSI. This PR can use this new ctest. Who will create the mgbf ctest?

Adding @ManuelPondeca-NOAA for awareness.

@ShunLiu-NOAA
Copy link
Contributor

@RussTreadon-NOAA Thank you. We will work with Ting to provide a regional DA test case.

@RussTreadon-NOAA
Copy link
Contributor

Great! Thank you @ShunLiu-NOAA and @TingLei-NOAA .

@RussTreadon-NOAA
Copy link
Contributor

@TingLei-daprediction , what is the status of this PR?

@TingLei-NOAA
Copy link
Contributor

TingLei-NOAA commented Aug 5, 2024 via email

@RussTreadon-NOAA
Copy link
Contributor

@TingLei-NOAA , if there is no timeline for actively working on this PR, I suggest that it be closed and reopened later when a path forward is worked out.

Attention @ShunLiu-NOAA

@TingLei-daprediction
Copy link
Contributor Author

As discussed in the above, this PR would be closed for being now and be re-opened when the mgbf ctest is ready

@RussTreadon-NOAA
Copy link
Contributor

Since this is an obvious error which is trivial to fix, let's get this fix into develop. We still need a mgbf ctest but that can be the subject of a new issue.

@TingLei-NOAA, if you agree that the change is correct, would you please approve this PR. @TingLei-NOAA , who else can peer review this PR?

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.

Change is correct. Integer variables should be declared as integer.

Approve.

@RussTreadon-NOAA
Copy link
Contributor

@MiodragRancic-NOAA , @dkokron found a bug in src/mgbf/mg_input.f90. Integer variables are declared as real variables. This PR correctly declares the variables as integers.

Do you have time to review this PR? We need one more peer review to merge it into develop.

@RussTreadon-NOAA
Copy link
Contributor

@TingLei-NOAA , who do you recommend as the second per reviewer for this PR?

@TingLei-NOAA
Copy link
Contributor

@RussTreadon-NOAA I think @GangZhao-NOAA will be a good reviewer for this PR. Thanks

@RussTreadon-NOAA
Copy link
Contributor

Thank you @TingLei-NOAA . @GangZhao-NOAA added as a peer reviewer.

Copy link
Contributor

@GangZhao-NOAA GangZhao-NOAA left a comment

Choose a reason for hiding this comment

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

The integer variable should be declared as integer type. The modifications are necessary, and approved by me.

@RussTreadon-NOAA RussTreadon-NOAA merged commit e82365d into NOAA-EMC:develop Aug 6, 2024
8 checks passed
DavidHuber-NOAA added a commit to DavidHuber-NOAA/GSI that referenced this pull request Sep 6, 2024
* origin/develop:
  Move to contrib spack-stack on Jet (NOAA-EMC#787)
  a quick workaround for increasing the mpi task numbers on orion for ctest :: rrfs_3denvar_rdasens  (NOAA-EMC#788)
  Recover the capability of handling model fields from operation gfs.v16.3 (NOAA-EMC#785)
  fix a bug in deter_sfc_gmi (NOAA-EMC#781)
  add safeguard to thompson_reff (NOAA-EMC#779)
  Fix incorrect usage of real(i_kind) in mg_input.f90  (NOAA-EMC#760)
  Transition to Thompson Microphysics for Microwave All-sky Assimilation (NOAA-EMC#743)
  Format changes for EUMETSAT metop-sg and CADS debug fix (NOAA-EMC#773)
  Update global_4denvar and global_enkf ctests to reflect GFS v17 (NOAA-EMC#774)
  fix for cris-fsr memory corruption (NOAA-EMC#767)
  Gnssrwnd1.0 (NOAA-EMC#747)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

real loop indexes used in mbpf
5 participants