search

NOAA-EMC / GSI

Gridpoint Statistical Interpolation
GNU Lesser General Public License v3.0
66 stars 151 forks source link

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

Closed TingLei-daprediction closed 3 months ago

TingLei-daprediction commented 5 months ago

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

RussTreadon-NOAA commented 5 months ago

@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 commented 5 months ago

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

RussTreadon-NOAA commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

RussTreadon-NOAA commented 5 months ago

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

RussTreadon-NOAA commented 3 months ago

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

TingLei-NOAA commented 3 months ago

@Russ Treadon - NOAA Federal @.***> When we began preparing a ctest for mgbf with GSI, I learned that the current implementation of mgbf won't guarantee the reproducibility using different mpi process numbers. As far as I know, there is no specific timelines, yet, for when that capability would be available with an upgraded MGBF package. I will come back with more updates when any specific plan is made.

Ting Lei

Physical Scientist, Contractor with Lynker in support of

EMC/NCEP/NWS/NOAA

5830 University Research Ct., Cubicle 2765

College Park, MD 20740

@.***

301-683-3624

On Mon, Aug 5, 2024 at 5:56 AM RussTreadon-NOAA @.***> wrote:

@TingLei-daprediction https://github.com/TingLei-daprediction , what is the status of this PR?

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/GSI/pull/760#issuecomment-2268673930, or unsubscribe https://github.com/notifications/unsubscribe-auth/APEFS7CG26KXY7A26NXHZY3ZP5D5RAVCNFSM6AAAAABJYP2AF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRYGY3TGOJTGA . You are receiving this because you were mentioned.Message ID: @.***>

RussTreadon-NOAA commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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?

RussTreadon-NOAA commented 3 months ago

@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 commented 3 months ago

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

TingLei-NOAA commented 3 months ago

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

RussTreadon-NOAA commented 3 months ago

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