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

Fortran standards bug-fixes #619

Merged
merged 22 commits into from Feb 13, 2024
Merged

Fortran standards bug-fixes #619

merged 22 commits into from Feb 13, 2024

Conversation

mjs2369
Copy link
Contributor

@mjs2369 mjs2369 commented Jan 10, 2024

Description:

This PR fixes multiple compile time errors related to fortran standards. For more details on the individual bugs, see the issue pages listed below.

Description of changes:
#351
DART/models/wrf/experiments/Radar/IC/sounding_perturbation/pert_sounding_module.f90:
Changed the random seed variables (iseed1 and iseed2) from real to integer types

#594 / #601

  • Replaced the use of iargc and getarg with Fortran instrisics COMMAND_ARGUMENT_COUNT() and GET_COMMAND_ARGUMENT() in all possible locations in the code
  • Removed declaration of the variable for iargc where unused in the code
  • Removed the f2kcli module and all uses of it in the DART code. This module defined its own subroutines for COMMAND_ARGUMENT_COUNT() and GET_COMMAND_ARGUMENT() that called iargc and getarg

#599
DART/models/wrf_hydro/noah_hydro_mod.f90:
Simplifies the implementation for summing the number of elements in the CH_NETRT array that are greater than or equal to 0 by using the count function

#352

  • Added specific mkmf.templates for the observation converters AIRS and quikscat, since they use specific configurations with the HDF and/or RTTOV libraries. Versions created for both the gfortran and ifort compilers. The templates that use gfortran make use of the -fallow-argument-mismatch flag to fix type mismatch errors in JPL code
  • NSIDC no longer produces the error described with the updated mkmf.template.rttov.gfortran file that will be merged with the rttov-clouds branch that makes use of the HDF5 library
  • The errors produced in the build for GSI2DART have been determined to be out of the scope of the fortran standards update as the entirety of the MPI implementation in this code needs refactored/fixed.

Fixes issue

Fixes #351 #352 #594 #601 #599

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Documentation changes needed?

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.

Tests

#351
Compiled successfully with gfortran 12.1.0 (the error was replicated with this version of gfortran before changes were made)

Compiled and run with intel, fixing the value of the seeds and comparing with the output on main. Due to changing the seeds from reals to ints, the changes are not bitwise with a fixed seed. Previous experiments that used real types for the seeds will not produce the same output when using the same value fixed for the seeds as integers.

Two executions with integer types for the seeds produce identical output when fixing the value of the seed.

#594 / #601
Compiled all of the following with cce and gfortran with full debug flags.

models/wrf/WRF_DART_utilities/add_pert_where_high_refl - add_pert_where_high_refl was run and produced correct outputs
models/wrf/WRF_DART_utilities/advance_cymdh - Executed the program, which was bitwise identical with main
models/wrf/WRF_DART_utilities/grid_refl_obs
models/wrf/WRF_BC/update_wrf_bc and models/wrf/WRF_BC/pert_wrf_bc
models/wrf/WRF_BC/update_wrf_bc and models/wrf/WRF_BC/update_wrf_bc
observations/obs_converters/AVISO/convert_aviso.f90
observations/obs_converters/NCEP/prep_bufr/convert_bufr/arg_test.f and observations/obs_converters/NCEP/prep_bufr/convert_bufr/stat_test.f:
observations/obs_converters/NCEP/prep_bufr/convert_bufr/arg_test.f

Note that observations/obs_converters/NCEP/prep_bufr/convert_bufr/grabbufr.f will not compile due to a separate issue.

#595
Both Noah and wrf_hydro compile without error with cce and gfortran with full debug flags
A test case is needed in order to run either model

#352
All observation converters (with the exception of GSI2DART, see above) compiled successfully using new mkmf.template files for both gfortran and intel

Checklist for merging

  • Updated changelog entry
  • Documentation updated
  • Update conf.py

Checklist for release

  • Merge into main
  • Create release from the main branch with appropriate tag
  • Delete feature-branch

Testing Datasets

  • Dataset needed for testing
  • Dataset download instructions included
  • No dataset needed

To execute a program that uses the noah_hydro_mod, a test case will need to be provided for for either Noah or wrf_hydro

@mjs2369 mjs2369 marked this pull request as draft January 10, 2024 20:02
@hkershaw-brown
Copy link
Member

@mjs2369 this is good stuff.
I'd like put this in v11 and as v10.X.X tag also.
Let's chat about this.
Cheers,
Helen

@mjs2369
Copy link
Contributor Author

mjs2369 commented Jan 10, 2024

I'd like put this in v11 and as v10.X.X tag also

@hkershaw-brown would you like me to go ahead and take this off draft then and move the outstanding issues (#352 and #595) to a later pull request?

@hkershaw-brown
Copy link
Member

@mjs2369 no, no rush, no need to split the request, the fortran standards fixes can go in together as one pull request.
I just think it will benefit us in the future for comparing versions of dart it will be useful to have a v10 that is compatible with the latest compilers. This note is to remind us (me) to make a v10.10.2 when we put this in to v11.

…odule defined its own subroutines for COMMAND_ARGUMENT_COUNT() and GET_COMMAND_ARGUMENT() that called iargc and getarg.

The only files that used this module were models/wrf/WRF_DART_utilities/add_pert_where_high_refl.f90 and DART/models/wrf/WRF_DART_utilities/grid_refl_obs.f90
…ARGUMENT_COUNT() and GET_COMMAND_ARGUMENT() in all possible locations in the code (models/wrf/WRF_DART_utilities/advance_cymdh.f90, observations/obs_converters/AVISO/convert_aviso.f90, and multiple files in observations/obs_converters/NCEP/prep_bufr/convert_bufr/)

Removed declaration of the variable for iargc where unused in the code (models/wrf/WRF_BC/pert_wrf_bc.f90 and models/wrf/WRF_BC/update_wrf_bc.f90)
…he CH_NETRT array that are greater than or equal to 0 by using the count function
…nding_module (wrf) from real to integer types
@hkershaw-brown
Copy link
Member

@mjs2369 I'm rebasing this pull request on v11

mjs2369 and others added 4 commits January 30, 2024 13:30
…and quikscat because they use specific configurations with the HDF and/or RTTOV libraries. Versions created for both the gfortran and ifort compilers; the templates that use gfortran make use of the -fallow-argument-mismatch flag to fix type mismatch errors in JPL code
… the necessary library flags, and point to the available mkmf.template files
@mjs2369 mjs2369 marked this pull request as ready for review February 1, 2024 00:52
Copy link
Member

@hkershaw-brown hkershaw-brown left a comment

Choose a reason for hiding this comment

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

Hi Marlee,

Looking good. I've left some suggestions (mostly removing large compiler doc sections from mkmf.

My only main concern is the prepbuffr code. This is not dart code, and has been fragile in the past. So I don't recommend we change this code without having extensive tests (similar to the JPL module code). This code is not built with the dart build system (mkmf, quickbuild). There's these two scripts for building.
https://github.com/NCAR/DART/blob/main/observations/obs_converters/NCEP/prep_bufr/install.sh
https://github.com/NCAR/DART/blob/main/observations/obs_converters/NCEP/prep_bufr/convert_bufr/convert_bufr.csh

Additionally, there is this
"Note that observations/obs_converters/NCEP/prep_bufr/convert_bufr/grabbufr.f will not compile due to a separate issue."
So I'd say we treat the prepbur code separately to DART code, and not have prepbuffr
changes in this pull request.

build_templates/mkmf.template.AIRS.gfortran Outdated Show resolved Hide resolved
build_templates/mkmf.template.AIRS.gfortran Outdated Show resolved Hide resolved
build_templates/mkmf.template.quikscat.gfortran Outdated Show resolved Hide resolved
build_templates/mkmf.template.quikscat.intel Outdated Show resolved Hide resolved
models/wrf_hydro/noah_hydro_mod.f90 Show resolved Hide resolved
observations/obs_converters/AIRS/README.rst Outdated Show resolved Hide resolved
observations/obs_converters/AVISO/convert_aviso.f90 Outdated Show resolved Hide resolved
observations/obs_converters/quikscat/QuikSCAT.rst Outdated Show resolved Hide resolved
observations/obs_converters/quikscat/QuikSCAT.rst Outdated Show resolved Hide resolved
@nancycollins
Copy link
Collaborator

My only main concern is the prepbuffr code. This is not dart code, and has been fragile in the past. So I don't recommend we change this code without having extensive tests (similar to the JPL module code). This code is not built with the dart build system (mkmf, quickbuild). There's these two scripts for building. https://github.com/NCAR/DART/blob/main/observations/obs_converters/NCEP/prep_bufr/install.sh https://github.com/NCAR/DART/blob/main/observations/obs_converters/NCEP/prep_bufr/convert_bufr/convert_bufr.csh

Additionally, there is this "Note that observations/obs_converters/NCEP/prep_bufr/convert_bufr/grabbufr.f will not compile due to a separate issue." So I'd say we treat the prepbur code separately to DART code, and not have prepbuffr changes in this pull request.

i'd agree with this, as long as there's an open issue on updating and testing the prep_bufr code at some point. it's an important converter - it generates all the conventional obs used in the reanalysis and other atmospheric runs.

mjs2369 and others added 7 commits February 1, 2024 14:56
…ntation in QuikSCAT.rst

Co-authored-by: Helen Kershaw <20047007+hkershaw-brown@users.noreply.github.com>
Co-authored-by: Helen Kershaw <20047007+hkershaw-brown@users.noreply.github.com>
…the AIRS documentation will be cleaned/fixed in an upcoming pull request

Co-authored-by: Helen Kershaw <20047007+hkershaw-brown@users.noreply.github.com>
Co-authored-by: Helen Kershaw <20047007+hkershaw-brown@users.noreply.github.com>
Co-authored-by: Helen Kershaw <20047007+hkershaw-brown@users.noreply.github.com>
…tent with other templates and usable with macports

Co-authored-by: Helen Kershaw <20047007+hkershaw-brown@users.noreply.github.com>
@mjs2369
Copy link
Contributor Author

mjs2369 commented Feb 1, 2024

My only main concern is the prepbuffr code. This is not dart code, and has been fragile in the past. So I don't recommend we change this code without having extensive tests (similar to the JPL module code). This code is not built with the dart build system (mkmf, quickbuild). There's these two scripts for building. https://github.com/NCAR/DART/blob/main/observations/obs_converters/NCEP/prep_bufr/install.sh https://github.com/NCAR/DART/blob/main/observations/obs_converters/NCEP/prep_bufr/convert_bufr/convert_bufr.csh

Additionally, there is this "Note that observations/obs_converters/NCEP/prep_bufr/convert_bufr/grabbufr.f will not compile due to a separate issue." So I'd say we treat the prepbur code separately to DART code, and not have prepbuffr changes in this pull request.

I agree with this and @nancycollins comment. There is currently not an open issue on updating and testing the prep_bufr code, but I will make one and remove these changes from this PR.

mjs2369 and others added 3 commits February 1, 2024 18:50
…in a separate issue for updating and testing the prep_bufr code
Co-authored-by: Helen Kershaw <20047007+hkershaw-brown@users.noreply.github.com>
@hkershaw-brown
Copy link
Member

hkershaw-brown commented Feb 12, 2024

Derecho: mpif90 failing for gfortran gcc/13.2.0

hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ mpif90 hello.f90 
Error: A PrgEnv-* modulefile must be loaded.
hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ cat hello.f90 
program hello_world
use mpi

integer :: rank, comm_size, ierr, tag, status(MPI_STATUS_SIZE)

call mpi_init(ierr)
call mpi_comm_size(mpi_comm_world, comm_size, ierr)
call mpi_comm_rank(mpi_comm_world, rank, ierr)

print*, 'Hello from rank ', rank, ' of ', comm_size
call mpi_finalize(ierr)

end program 
hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ module list

Currently Loaded Modules:
  1) ncarenv/23.09 (S)   2) craype/2.7.23   3) gcc/13.2.0   4) ncarcompilers/1.0.0   5) hdf5/1.14.3   6) netcdf/4.9.2   7) cray-mpich/8.1.27

  Where:
   S:  Module is Sticky, requires --force to unload or purge

@hkershaw-brown
Copy link
Member

hkershaw-brown commented Feb 12, 2024

MITgcm failing for utilities::nc_check

/glade/derecho/scratch/hkershaw/build_everything/ifort.2024-02-12T0725/DART/models/MITgcm_ocean/model_mod.f90(25): error #6580: Name in only-list does not exist or is not accessible.   [NC_CHECK]
                                  logfileunit, get_unit, nc_check, do_output, to_upper, &\

This is failing on main, fixing it on this branch.

@hkershaw-brown
Copy link
Member

hkershaw-brown commented Feb 12, 2024

Derecho h4fc error:

h4fc -O -assume buffered_io -I/glade/u/apps/derecho/23.09/spack/opt/spack/netcdf/4.9.2/oneapi/2023.2.1/yzvj/include  -c	/glade/derecho/scratch/hkershaw/DART/pull_reqeuests/pull_619/DART.converters/assimilation_code/modules/utilities/types_mod.f90
[spack cc] ERROR: Spack compiler must be run from Spack! Input 'SPACK_ENV_PATH' is missing.
make: *** [Makefile:14: types_mod.o] Error 1

To work around this, not using h4fc and putting the linking flags in mkmf.template
-lmfhdf -ldf -lsz -ljpeg -lz -ltirpc

@hkershaw-brown
Copy link
Member

Hi Marlee, this looks great.
I've filed a ticket with CISL about the spack h4fc issue.
I think if it is not a quick fix (today), we can add the flags -lmfhdf -ldf -lsz -ljpeg -lz -ltirpc in the AIRS and quickscat mkmf.templates and take out the h4fc

@mjs2369
Copy link
Contributor Author

mjs2369 commented Feb 12, 2024

I've filed a ticket with CISL about the spack h4fc issue. I think if it is not a quick fix (today), we can add the flags -lmfhdf -ldf -lsz -ljpeg -lz -ltirpc in the AIRS and quickscat mkmf.templates and take out the h4fc

Sounds good, keep me posted.

Derecho: mpif90 failing for gfortran gcc/13.2.0

hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ mpif90 hello.f90 
Error: A PrgEnv-* modulefile must be loaded.
hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ cat hello.f90 
program hello_world
use mpi

integer :: rank, comm_size, ierr, tag, status(MPI_STATUS_SIZE)

call mpi_init(ierr)
call mpi_comm_size(mpi_comm_world, comm_size, ierr)
call mpi_comm_rank(mpi_comm_world, rank, ierr)

print*, 'Hello from rank ', rank, ' of ', comm_size
call mpi_finalize(ierr)

end program 
hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ module list

Currently Loaded Modules:
  1) ncarenv/23.09 (S)   2) craype/2.7.23   3) gcc/13.2.0   4) ncarcompilers/1.0.0   5) hdf5/1.14.3   6) netcdf/4.9.2   7) cray-mpich/8.1.27

@hkershaw-brown I've never seen this before, did they do some updates to the gcc module while it was down last week? I tried to replicate this error, but I don't even have gcc/13.2.0 available in my modules. Just 12.2.0, which doesn't give this error.

Would this hello code compile with 'use mpif08'?

@hkershaw-brown
Copy link
Member

gcc/13 is the default for me when I log in.

hkershaw@derecho6:/glade/derecho/scratch/hkershaw/test_code/mpi$ module avail gcc

-------------------------------------------------------------- Compilers and Core Software ---------------------------------------------------------------
   gcc/12.2.0    gcc/13.2.0 (L,D)

  Where:
   L:  Module is loaded
   D:  Default Module

Yeah you're correct it will need mpif08 to compile, but it doesn't even get to compiling. mpif90 -show was giving me Error: A PrgEnv-* modulefile must be loaded.

I think CISL are working on it at the moment, since there's a slightly different error.

hkershaw@derecho6:/glade/derecho/scratch/hkershaw/test_code/mpi$ mpif90 -show
Error:
Unable to determine compiler version.
Make sure that a gnu module is loaded and that GNU_VERSION is defined

@mjs2369
Copy link
Contributor Author

mjs2369 commented Feb 12, 2024

gcc/13 is the default for me when I log in.

hkershaw@derecho6:/glade/derecho/scratch/hkershaw/test_code/mpi$ module avail gcc

-------------------------------------------------------------- Compilers and Core Software ---------------------------------------------------------------
   gcc/12.2.0    gcc/13.2.0 (L,D)

  Where:
   L:  Module is loaded
   D:  Default Module

So weird, I don't have it.

masmith@derecho5:/glade/derecho/scratch/masmith> module avail gcc

------------------------------------------------------- Compilers and Core Software -------------------------------------------------------
   gcc/12.2.0 (L)

  Where:
   L:  Module is loaded

@hkershaw-brown
Copy link
Member

So weird, I don't have it.

masmith@derecho5:/glade/derecho/scratch/masmith> module avail gcc

------------------------------------------------------- Compilers and Core Software -------------------------------------------------------
   gcc/12.2.0 (L)

  Where:
   L:  Module is loaded

guess it is cool kids only.

@hkershaw-brown
Copy link
Member

Derecho: mpif90 failing for gfortran gcc/13.2.0

hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ mpif90 hello.f90 
Error: A PrgEnv-* modulefile must be loaded.
hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ cat hello.f90 
program hello_world
use mpi

integer :: rank, comm_size, ierr, tag, status(MPI_STATUS_SIZE)

call mpi_init(ierr)
call mpi_comm_size(mpi_comm_world, comm_size, ierr)
call mpi_comm_rank(mpi_comm_world, rank, ierr)

print*, 'Hello from rank ', rank, ' of ', comm_size
call mpi_finalize(ierr)

end program 
hkershaw@dec1763:/glade/derecho/scratch/hkershaw/test_code/mpi$ module list

Currently Loaded Modules:
  1) ncarenv/23.09 (S)   2) craype/2.7.23   3) gcc/13.2.0   4) ncarcompilers/1.0.0   5) hdf5/1.14.3   6) netcdf/4.9.2   7) cray-mpich/8.1.27

  Where:
   S:  Module is Sticky, requires --force to unload or purge

Update from CISL, gcc 13 mpi fixed on Derecho !

@hkershaw-brown
Copy link
Member

CISL fixed the spack for intel, and gcc/12

Copy link
Member

@hkershaw-brown hkershaw-brown left a comment

Choose a reason for hiding this comment

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

Great work Marlee, approved.

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.

argument mismatch wrf pert_model_sounding
3 participants