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

Resolve various subroutine argument mismatches #160

Merged
merged 41 commits into from
Mar 19, 2024

Conversation

DusanJovic-NOAA
Copy link

Starting with version 10 of GNU Fortran compiler, subroutine argument mismatches are flagged as errors, and must be silenced by using '-fallow-argument-mismatch' flag, which can potentially hide actual bugs, which is undesirable.

This PR fixes all such mismatches, mostly by switching to use mpi_f08 MPI module, which provides generic interfaces, using F90 version of various netcdf calls, or by passing correct type of arguments to w3lib calls.

The changes are tested by running full regression test of ufs-weather-model. See ufs-community/ufs-weather-model#1147

DusanJovic-NOAA and others added 30 commits March 24, 2022 14:45
Switch from 'use mpi' to 'use mpi_f08'
@grantfirl
Copy link
Collaborator

This looks fine to me, although it seems like CCPP framework code managers might like the MPI_f08 stuff modified? One question that I had was you've added ncerr = ... to the NetCDF calls, but it doesn't get checked anywhere within the routine or in the calling routine (it isn't even an argument in the subroutine). I don't know if it's beyond the scope of this PR to add a response to finding ncerr /= 0 or not.

@DusanJovic-NOAA
Copy link
Author

Regarding the check of the netcdf status code (error code), I looked at other routines where netcdf calls are used. In some cases retrun code is simply ignored, for example:

status = nf90_inq_dimid(ncid, 'nband', dimid)
status = nf90_inquire_dimension(ncid, dimid, len=nBandSW)
status = nf90_inq_dimid(ncid, 'nrghice', dimid)

In some cases error code is checked and local subroutine (netcdf_err) is called that aborts the program, with FATAL error message:

error=nf90_open(trim(filename), nf90_nowrite, ncid)
if (error /= nf90_noerr) call netcdf_err(error)

while in some other cases error code is checked, again local function (netcdf_check) is called, error message and flag are set and subroutine immediately returns.

if(.not.netcdf_check(nf90_inq_varid(ncid, vname, varid), &
errmsg, errflg, 'find id of '//trim(vname)//' var')) then
return
endif

It's not obvious what method of netcdf error checking is preferable.

Ideally there should be one method of checking and handling netcdf errors and it should be used everywhere.

@climbfuji
Copy link

@GeorgeGayno-NOAA Can you please review the changes in physics/Interstitials/UFS_SCM_NEPTUNE/sfcsub.F and any other files changed in this PR that UFS_UTILS may use?

@GeorgeGayno-NOAA
Copy link

@GeorgeGayno-NOAA Can you please review the changes in physics/Interstitials/UFS_SCM_NEPTUNE/sfcsub.F and any other files changed in this PR that UFS_UTILS may use?

UFS_UTILS only uses sfcsub.F (the global_cycle program). And these changes should not affect global_cycle. Thanks for asking.

@climbfuji
Copy link

@DusanJovic-NOAA Now that we've established that MPI f08 is a mandatory dependency of CCPP, can you make the following change please?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d0d26543..d94b17e9 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.3)
+cmake_minimum_required(VERSION 3.10)

 project(ccpp_physics
         VERSION 5.0.0
@@ -9,9 +9,11 @@ set(PACKAGE "ccpp-physics")
 set(AUTHORS "Grant Firl" "Dustin Swales" "Man Zhang" "Mike Kavulich" )

 #------------------------------------------------------------------------------
-# Set MPI flags for C/C++/Fortran
-if (MPI)
-  find_package(MPI REQUIRED C Fortran)
+# Set MPI flags for C/C++/Fortran with MPI F08 interface
+# DH* I BELIEVE WE NEED THE LANGUAGES  FORTRAN IN THE PROJECT DEFINITION IF WE WANT TO FIND MPI_C
+find_package(MPI REQUIRED C Fortran)
+if(NOT MPI_Fortran_HAVE_F08_MODULE)
+  message(FATAL_ERROR "MPI implementation does not support the Fortran 2008 mpi_f08 interface")
 endif()

 #------------------------------------------------------------------------------

Note my little comment in there (please remove ...). We should check if we need the C interface or not for the find_MPI call, and adjust the project definition accordingly?

Copy link
Collaborator

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

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

This all looks great. Thanks for making these changes.
(Just one comment on the CMakeLists, same as in the corresponding ccpp-framework PR).

CMakeLists.txt Outdated
@@ -8,6 +8,13 @@ project(ccpp_physics
set(PACKAGE "ccpp-physics")
set(AUTHORS "Grant Firl" "Dustin Swales" "Man Zhang" "Mike Kavulich" )

#------------------------------------------------------------------------------
# Set MPI flags for C/C++/Fortran with MPI F08 interface
find_package(MPI REQUIRED C Fortran)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -385,7 +385,7 @@ subroutine GFS_diagtoscreen_run (Model, Statein, Stateout, Sfcprop, Coupling,
nthreads, blkno, errmsg, errflg)

#ifdef MPI

Choose a reason for hiding this comment

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

The CCPP framework developers decided to remove the remaining ifdef MPI and associated build system logic to pass -DMPI in a follow-up PR, since these changes would make your PR even larger.

@zach1221
Copy link

Hi, @grantfirl . Testing is complete on ufs-wm PR #1147 . Please feel free to merge this PR.

@dustinswales dustinswales merged commit 9839680 into ufs-community:ufs/dev Mar 19, 2024
3 checks passed
@DusanJovic-NOAA DusanJovic-NOAA deleted the no_arg_mismatch branch March 28, 2024 13:33
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.

7 participants