Skip to content

Commit

Permalink
Consistent double precision definitions (#2099)
Browse files Browse the repository at this point in the history
TYPE: bug fix

KEYWORDS: double precision, configuration, make, cmake

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem:
Currently, the source code has multiple preprocessor definitions for
controlling double precision usage (1). Likewise, there are multiple
parameter definitions in the IO code for the `WRF_REAL` value (2).

Examples of (1) :
```
#if ( RWORDSIZE == 8 )
#if ( RWORDSIZE == DWORDSIZE )
#if ( DWORDSIZE == 8 && RWORDSIZE == 8 )
```

Because there is no definitive define for querying double precision, it
has been left as an exercise to the contributor to formulate an adequate
conditional. While the above and other such forms work, they are not
consistent and can be confusing as to the intent.

Examples of (2) in a directory with option `-r8` :
```bash
 grep -RiE "WRF_REAL.*=.*[0-9]+" | sort -u
# original version
external/ioapi_share/wrf_io_flags.h:      integer, parameter  :: WRF_REAL                             = 104
external/io_grib1/io_grib1.f90:      integer, parameter  :: WRF_REAL                             = 104
external/io_grib_share/io_grib_share.f90:      integer, parameter  :: WRF_REAL                             = 104
external/io_int/diffwrf.f:      integer, parameter  :: WRF_REAL                             = 104
external/io_int/io_int.f:      integer, parameter  :: WRF_REAL                             = 105
external/io_netcdf/wrf_io.f:      integer, parameter  :: WRF_REAL                             = 104
frame/module_io.f90:      integer, parameter  :: WRF_REAL                             = 105
frame/module_quilt_outbuf_ops.f90:      integer, parameter  :: WRF_REAL                             = 105
# modified version
# inc/wrf_io_flags.h:      integer, parameter  :: WRF_REAL                             = 105
main/real_em.f90:      integer, parameter  :: WRF_REAL                             = 105
share/input_wrf.f90:      integer, parameter  :: WRF_REAL                             = 105
share/mediation_integrate.f90:      integer, parameter  :: WRF_REAL                             = 105
share/output_wrf.f90:      integer, parameter  :: WRF_REAL                             = 105
share/track_input.f90:      integer, parameter  :: WRF_REAL                             = 105
share/wrf_bdyin.f90:      integer, parameter  :: WRF_REAL                             = 105
share/wrf_bdyout.f90:      integer, parameter  :: WRF_REAL                             = 105
share/wrf_tsin.f90:      integer, parameter  :: WRF_REAL                             = 105
var/da/da_main/da_update_firstguess.inc:  wrf_real=104
```

Across many different preprocessed files, there appears to be two values
of `WRF_REAL` which could lead to undesired behavior when interfacing
between different sections of code. This issue arises from the `sed`
command in `external/ioapi_share/makefile` where `wrf_io_flags.h` is
changed in the `inc/` folder _only_, and thus anything including
`external/ioapi_share` first has one definition whilst anything
including `inc/` has the changed value.

While (2) may seem like an entirely separate problem from (1) they are
interrelated. `wrf_io_flags.h` already partially contains the necessary
logic to control whether to use `104` or `105` when double precision
promotion is requested. The current logic is not being used correctly
fully as it uses a totally different (and undefined) form of double
precision query :
```fortran
#ifdef PROMOTE_FLOAT
  integer, parameter :: WRF_FLOAT = WRF_DOUBLE
#else
  integer, parameter :: WRF_FLOAT = WRF_REAL
#endif
```
The end result will always be `WRF_FLOAT = WRF_REAL` regardless of `-r8`
option since `PROMOTE_FLOAT` is not defined anywhere in the
configuration / compilation logic. However, `WRF_FLOAT` _happens to be
used correctly_ since the `sed` rewrite has changed `WRF_REAL` to `105`
(effectively the same as `WRF_FLOAT = WRF_DOUBLE`). This only works
because `WRF_FLOAT` is exclusively used only in files that access the
`inc/wrf_io_flags.h` rewritten file and not the
`external/ioapi_share/wrf_io_flags.h` one. Furthermore, aside from
`io_int.F`, no code that contains the `105` value utilizes `WRF_REAL`
```bash
# Looking for any incorrect usage of WRF_FLOAT in files with 104 value
# we're really only concerned with unique statements of computational code
grep -RiEl "WRF_REAL.*=.*104" | sort -u  | xargs -i grep -iH WRF_FLOAT {} | sort -u
external/ioapi_share/wrf_io_flags.h:      integer, parameter  :: WRF_FLOAT=WRF_DOUBLE
external/io_grib1/io_grib1.f90:      integer, parameter  :: WRF_FLOAT=WRF_REAL
external/io_grib_share/io_grib_share.f90:      integer, parameter  :: WRF_FLOAT=WRF_REAL
external/io_int/diffwrf.f:      integer, parameter  :: WRF_FLOAT=WRF_REAL
external/io_netcdf/wrf_io.f:      integer, parameter  :: WRF_FLOAT=WRF_REAL
# No usage of bad value, only the include declaration shows up
```

```bash
# Look for usage of WRF_REAL where its value has been changed to 105 thus
# leading to ambiguous definitions
# Exclude declarations for brevity 
grep -RiEl "WRF_REAL.*=.*105" | sort -u  | xargs -i grep -iH WRF_REAL {} | sort -u | grep -vE "integer, parameter[ ]*:: WRF_"
external/io_int/io_int.f:            CALL wrf_message('io_int.F90: ext_int_read_field: types other than WRF_REAL not supported yet')
external/io_int/io_int.f:  IF      ( FieldType .EQ. WRF_REAL .OR. FieldType .EQ. WRF_DOUBLE) THEN
external/io_int/io_int.f:          IF      ( FieldType .EQ. WRF_REAL ) THEN
external/io_int/io_int.f:    IF      ( FieldType .EQ. WRF_REAL ) THEN
# These are character strings
main/real_em.f90:      IF ( config_flags%auxinput1_inname(1:8) .NE. 'wrf_real' ) THEN
share/input_wrf.f90:              ( config_flags%auxinput1_inname(1:8) .EQ. 'wrf_real' ) ) ) THEN
```

Solution:
To reduce the overall complexity of various define constructs _and_ IO
inconsistencies a singular define `DOUBLE_PRECISION` can be introduced
specifically meant to inform sections of code whether double precision
promotion has been requested.

Adding "one more define" may not sound appealing at first, but it does
carry some benefits :
* Firstly, it does not require the user to hard code values of `4` or
`8` for single or double precision, respectively, where those are
already defined by `NATIVE_RWORDSIZE` and `DWORDSIZE`.

* Furthermore, rather than use `#if (RWORDSIZE == DWORDSIZE)` logic can
be simplified to a more coherent statement of `#ifdef DOUBLE_PRECISION`
which is more readable and better understood in intent.

* Finally, reducing duplication of defines to do the same thing is good
practice _and_ limiting defines to control one thing only avoiding
co-opting for multiple roles is *also* good practice. The first part
sounds counter to a new define but `RWORDSIZE` and `DOUBLE_PRECISION`
serve two different functions - the former to tell us the numeric size
of real data in bytes and the latter to tell us if alternate logic
should be used. An alternative solution to only use a single define
would be to have `DOUBLE_PRECISION` control an `rkind` setting
`RWORDSIZE` much like `kind_phys` for ccpp

For areas where `WRF_REAL` was used with a value of `105` when `-r8` is
used (`io_int.F`), to maintain previous behavior the value should be
changed to `WRF_FLOAT`. Instead of using `sed` to rewrite the file,
`#ifdef PROMOTE_FLOAT` will use the valid `DOUBLE_PRECISION` define to
switch control of `WRF_FLOAT` to `WRF_DOUBLE` if defined, or `WRF_REAL`
if not.

For the CMake build, `DOUBLE_PRECISION` needs to be added to the
defines. No other changes necessary.

To reduce the complexity of these changes, `wrf_io_flags.h` is still
copied out to `inc/`, this may be addressed separately.

TESTS CONDUCTED: 
1. Single and double precision builds should compile without issue and
have effectively the same logic as before
  • Loading branch information
islas authored Jan 11, 2025
1 parent 5b09725 commit 7042598
Show file tree
Hide file tree
Showing 39 changed files with 252 additions and 248 deletions.
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,9 @@ list( APPEND PROJECT_COMPILE_DEFINITIONS_OPTIONS
# Only define if set, this is to use #ifdef/#ifndef preprocessors
# in code since cmake cannot handle basically any others :(
# https://gitlab.kitware.com/cmake/cmake/-/issues/17398
if ( ${USE_DOUBLE} )
list( APPEND PROJECT_COMPILE_DEFINITIONS_OPTIONS DOUBLE_PRECISION )
endif()
if ( ${ENABLE_CHEM} )
list( APPEND PROJECT_COMPILE_DEFINITIONS_OPTIONS WRF_CHEM=1 )
if ( ${ENABLE_KPP} )
Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ em_real : wrf
ln -sf ../../run/ishmael-qi-qr.bin . ; \
ln -sf ../../run/BROADBAND_CLOUD_GODDARD.bin . ; \
ln -sf ../../run/STOCHPERT.TBL . ; \
if [ $(RWORDSIZE) -eq 8 ] ; then \
if [ -n "$(DOUBLE_PRECISION)" ] ; then \
ln -sf ../../run/ETAMPNEW_DATA_DBL ETAMPNEW_DATA ; \
ln -sf ../../run/ETAMPNEW_DATA.expanded_rain_DBL ETAMPNEW_DATA.expanded_rain ; \
ln -sf ../../run/RRTM_DATA_DBL RRTM_DATA ; \
Expand Down Expand Up @@ -677,7 +677,7 @@ em_real : wrf
ln -sf ../../run/ishmael-qi-qr.bin . ; \
ln -sf ../../run/BROADBAND_CLOUD_GODDARD.bin . ; \
ln -sf ../../run/STOCHPERT.TBL . ; \
if [ $(RWORDSIZE) -eq 8 ] ; then \
if [ -n "$(DOUBLE_PRECISION)" ] ; then \
ln -sf ../../run/ETAMPNEW_DATA_DBL ETAMPNEW_DATA ; \
ln -sf ../../run/ETAMPNEW_DATA.expanded_rain_DBL ETAMPNEW_DATA.expanded_rain ; \
ln -sf ../../run/RRTM_DATA_DBL RRTM_DATA ; \
Expand Down
10 changes: 10 additions & 0 deletions arch/Config.pl
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
$sw_compileflags="";
$sw_opt_level="";
$sw_rwordsize="\$\(NATIVE_RWORDSIZE\)";
$sw_promotion="";
$sw_rttov_flag = "" ;
$sw_rttov_inc = "" ;
$sw_rttov_path = "" ;
Expand Down Expand Up @@ -228,6 +229,10 @@
{
$sw_config_line=substr( $ARGV[0], 13 ) ;
}
if ( substr( $ARGV[0], 1, 6 ) eq "rword=" )
{
$sw_rwordsize=substr( $ARGV[0], 7 ) ;
}
shift @ARGV ;
}

Expand Down Expand Up @@ -316,6 +321,10 @@
}

$sw_rwordsize = "8" if ( $sw_wrfplus_core eq "-DWRFPLUS=1" );
if ( $sw_rwordsize eq "8" )
{
$sw_promotion = "-DDOUBLE_PRECISION" ;
}

# A separately-installed ESMF library is required to build the ESMF
# implementation of WRF IOAPI in external/io_esmf. This is needed
Expand Down Expand Up @@ -634,6 +643,7 @@
$_ =~ s/CONFIGURE_LDFLAGS/$sw_ldflags/g ;
$_ =~ s/CONFIGURE_COMPILEFLAGS/$sw_compileflags/g ;
$_ =~ s/CONFIGURE_RWORDSIZE/$sw_rwordsize/g ;
$_ =~ s/CONFIGURE_PROMOTION/$sw_promotion/g ;
$_ =~ s/CONFIGURE_FC/$sw_time $sw_fc/g ;
$_ =~ s/CONFIGURE_CC/$sw_cc/g ;
$_ =~ s/CONFIGURE_COMMS_LIB/$sw_comms_lib/g ;
Expand Down
Loading

0 comments on commit 7042598

Please sign in to comment.