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

Release/gfsda.v16.3.0 warnings #464

Merged
merged 2 commits into from
Aug 28, 2022

Conversation

emilyhcliu
Copy link
Contributor

@emilyhcliu emilyhcliu commented Aug 26, 2022

Per NCO's request for gfs.v16.3.0 code delivery, we were asked to fix a few warning messages from the compilation of the GSI code. The warnings are listed as the following:

/gsi.fd/util/Conventional_Monitor/nwprod/conmon_shared/sorc/conmon_grads_lev.fd/maingrads_lev.f90(20): warning #6717: This name has not been given an explicit type.   [ISCATER]
/gsi.fd/util/Conventional_Monitor/nwprod/conmon_shared/sorc/conmon_grads_sfctime.fd/maingrads_sfctime.f90(23): warning #6717: This name has not been given an explicit type.   [IFILEO]
/gsi.fd/util/Conventional_Monitor/nwprod/conmon_shared/sorc/conmon_grads_sfctime.fd/maingrads_sfctime.f90(20): warning #6717: This name has not been given an explicit type.   [IGRADS]
/gsi.fd/util/Conventional_Monitor/nwprod/conmon_shared/sorc/conmon_grads_sig.fd/maingrads_sig.f90(20): warning #6717: This name has not been given an explicit type.   [ISCATER]

/gsi.fd/src/gsi/stub_wrf_binary_interface.f90(38): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [CTPH0]
/gsi.fd/src/gsi/stub_wrf_binary_interface.f90(30): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [CTPH0]
/gsi.fd/src/gsi/stub_wrf_binary_interface.f90(38): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [STPH0]
/gsi.fd/src/gsi/stub_wrf_binary_interface.f90(30): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [STPH0]
/gsi.fd/src/gsi/stub_wrf_binary_interface.f90(38): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [TLM0]
/gsi.fd/src/gsi/stub_wrf_binary_interface.f90(30): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [TLM0]
/gsi.fd/src/gsi/stub_wrf_netcdf_interface.f90(31): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [CTPH0]
/gsi.fd/src/gsi/stub_wrf_netcdf_interface.f90(31): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [STPH0]
/gsi.fd/src/gsi/stub_wrf_netcdf_interface.f90(31): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value.   [TLM0]

There are two sets of warnings. The first set comes from WRF binary and netcdf intricate. Three variables: ctph0, stph0, and tlm0, were not initialized.
The solution is to give them zeros as initial value.

The second set of warning comes from the conventional data monitoring code. Some of the variables were not declared properly.
The solution is to declare the variables before they are used.

The warnings went away after the proposed fixes.

@aerorahul
Copy link
Contributor

@emilyhcliu
I am not the person to review these as I can't comment on their impacts.
I will note that, this PR and any other PR that goes into release/gfsda.v16.3.0, please collect them for develop when the dust settles.

@emilyhcliu
Copy link
Contributor Author

@aerorahul Please review the warnings from the WRF netcdf/binary interface. Your review and input will be helpful since you were the author of that routine.

@RussTreadon-NOAA
Copy link
Contributor

Source code in Conventional_Monitor should not be part of the release/gfsda.v16.3.0 build since this package is not used in operations. The forked release/gfsda.v16.3.0 modifies the BUILD_UTIL_MON section of util/CMakeLists.txt so that it reads

if(BUILD_UTIL_MON)
# add_subdirectory(Conventional_Monitor)
  add_subdirectory(Ozone_Monitor)
  add_subdirectory(Radiance_Monitor)
endif()

With this change, Conventional_Monitor is by-passed in the release/gfsda.v16.3.0 build.

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

I approve the changes in the stubs.
I cannot say about the changes in the monitoring codes.

Comment on lines +37 to +39
ctph0 = zero
stph0 = zero
tlm0 = zero
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a stub. Meaning, this code exists solely for compilation to succeed in making the handshake.
This routine will not be called at runtime, and if it is, then something is wrong elsewhere.
The values zero are fine here and possibly serve for suppressing compilation warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no run-time errors from this stub routine. They were compilation warnings. NCO wants these warnings to be fixed. Thanks for your review.

Comment on lines +49 to +51
ctph0 = zero
stph0 = zero
tlm0 = zero
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above.

Comment on lines +39 to +41
ctph0 = zero
stph0 = zero
tlm0 = zero
Copy link
Contributor

Choose a reason for hiding this comment

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

and here.

@RussTreadon-NOAA
Copy link
Contributor

Regarding the changes to src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90, has release/gfsda.v16.3.0 been built and run with the proposed changes to confirm no change in cycled results? This should be the case. Has this been confirmed.

A check of the history of src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90 in release/gfsda.v16.3.0 goes back to Mark Potts

@emilyhcliu
Copy link
Contributor Author

@RussTreadon-NOAA Thanks for your suggestion. I will comment out the following line in util/CmakeLists.txt:

# add_subdirectory(Conventional_Monitor)

@RussTreadon-NOAA
Copy link
Contributor

@emilyhcliu , please do NOT comment out

add_subdirectory(Conventional_Monitor)

in this PR. It has already been commented out in PR #456. Please consider merging PR #456 before merging this PR into release/gfsda.v16.3.0

@emilyhcliu
Copy link
Contributor Author

@emilyhcliu , please do NOT comment out

add_subdirectory(Conventional_Monitor)

in this PR. It has already been commented out in PR #456. Please consider merging PR #456 before merging this PR into release/gfsda.v16.3.0

@RussTreadon-NOAA Great! Thanks for letting me know. I will undo the fix in the CMakeLists.txt
When you are ready, lets merge PR #456 first.

One question: should I still merge the code changes to fix the variable problem declaration in this PR for gfsda.v16.3.0?

@emilyhcliu
Copy link
Contributor Author

Regarding the changes to src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90, has release/gfsda.v16.3.0 been built and run with the proposed changes to confirm no change in cycled results? This should be the case. Has this been confirmed.

A check of the history of src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90 in release/gfsda.v16.3.0 goes back to Mark Potts

Not yet. I will run a cycled experiment to verify.

@RussTreadon-NOAA
Copy link
Contributor

@emilyhcliu , please do NOT comment out
add_subdirectory(Conventional_Monitor)
in this PR. It has already been commented out in PR #456. Please consider merging PR #456 before merging this PR into release/gfsda.v16.3.0

@RussTreadon-NOAA Great! Thanks for letting me know. I will undo the fix in the CMakeLists.txt When you are ready, lets merge PR #456 first.

One question: should I still merge the code changes to fix the variable problem declaration in this PR for gfsda.v16.3.0?

Isn't NCO requiring that we fix the build WARNING messages? If so, these changes must go into release/gfsda.v16.3.0.

@RussTreadon-NOAA
Copy link
Contributor

Regarding the changes to src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90, has release/gfsda.v16.3.0 been built and run with the proposed changes to confirm no change in cycled results? This should be the case. Has this been confirmed.
A check of the history of src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90 in release/gfsda.v16.3.0 goes back to Mark Potts

Not yet. I will run a cycled experiment to verify.

Building and executing gsi.x for a single case is sufficient.

@emilyhcliu
Copy link
Contributor Author

Regarding the changes to src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90, has release/gfsda.v16.3.0 been built and run with the proposed changes to confirm no change in cycled results? This should be the case. Has this been confirmed.
A check of the history of src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90 in release/gfsda.v16.3.0 goes back to Mark Potts

Not yet. I will run a cycled experiment to verify.

Second thought on running an experiment to verify. These are changes for regional model (WRF). I have no means to verify the changes. Since this is a stub as mentioned @aerorahul , the changes of adding initial values do not impact run time at all. I think the changes should be OK.

@emilyhcliu
Copy link
Contributor Author

Regarding the changes to src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90, has release/gfsda.v16.3.0 been built and run with the proposed changes to confirm no change in cycled results? This should be the case. Has this been confirmed.
A check of the history of src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90 in release/gfsda.v16.3.0 goes back to Mark Potts

Not yet. I will run a cycled experiment to verify.

Building and executing gsi.x for a single case is sufficient.

OK, I will run a global case to make sure the update gsi.x does not change results.

@RussTreadon-NOAA
Copy link
Contributor

Regarding the changes to src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90, has release/gfsda.v16.3.0 been built and run with the proposed changes to confirm no change in cycled results? This should be the case. Has this been confirmed.
A check of the history of src/gsi/stub_wrf_binary_interface.f90 and src/gsi/stub_wrf_netcdf_interface.f90 in release/gfsda.v16.3.0 goes back to Mark Potts

Not yet. I will run a cycled experiment to verify.

Building and executing gsi.x for a single case is sufficient.

OK, I will run a global case to make sure the update gsi.x does not change results.

Thank you. I agree that these changes should not impact global results. A quick test to confirm this is crossing our t's and dotting our i's.

@RussTreadon-NOAA
Copy link
Contributor

Adding @EdwardSafford-NOAA for awareness. Conventional_Monitor no longer resides in NOAA-EMC/GSI. It's in its own repo, NOAA-EMC/GSI-Monitor.

@emilyhcliu
Copy link
Contributor Author

I ran a cycled experiment for 3 cycles with the changes of initializing ctpho, stphi and stlm0 to zero in stub_wrf_binary_interface.f90 and stb_wrf_netcdf_interface.f90.

Comparing the control and experiment, there are no impacts on the results regarding the analysis and the downstream forecasts. The gsistat files, gnorms of the analysis, and all forecast pgb files are identical for all three cycles.

For enkf part, the observation statistics in the enkf stat files were compared between the control and the experiment for the third cycle. They are identical. The forecast files for ensemble means are also identical. The selected ensemble member forecast files were checked. They were also identical.

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.

As noted, changes to Conventional_Monitor routines need to be committed to GSI-Monitor repo. Conventional_Monitor package no longer resides in GSI repo. Given the change to util/CMakeLists.txt in PR #456, Conventional_Monitor is no longer built in release/gfsda.v16.3.0

The changes to the two gsi source code files have been tested in the gsi.x and, as expected, found to not alter results. The compilation warnings indicate a true oversight in the code. The intent(out) variables should be given a value. This PR satisfies an NCO implementation requirement that code compilation not generate warning messages.

Approve changes.

@emilyhcliu emilyhcliu merged commit 148b7d8 into release/gfsda.v16.3.0 Aug 28, 2022
@emilyhcliu emilyhcliu deleted the release/gfsda.v16.3.0_warnings branch September 2, 2022 22:44
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.

3 participants