Bug fix for unit conversion error in ccpp_prebuild.py (variables that…

… are slices of a n+1 dimensional array have wrong allocation) (#600)

## Description

1. Bug fix in `scripts/mkstatic.py`: define `dim_string_allocate` so
that temporary (group cap) variables of rank `n` that correspond to a
slice of an `n+1` dimensional array have the right dimensions in the
assignment calls and subroutine call lists. See the inline documentation
added in `mkstatic.py` around line 1476 for more information.

2. Addition of test `test_prebuild/test_unit_conv` to test for the above
and to test for the `capgen` issue reported in
#594 (Unit conversion bug
when variable is used by two different schemes with different units).
Note that `ccpp_prebuild.py` did not suffer from this bug, but I wanted
to make sure that we test for it.

3. In the development of the new test `test_unit_conv`, I discovered
that we should declare all incoming host variables in the group caps as
`target`. This is because it is tricky in prebuild to determine that a
parent variable `bar` needs to have the `target` attribute in case a
slice of it has the active attribute. This is a bit of an edge case, but
I believe this additional attribute is safe. I will run full UFS RTs
with this PR to check if the results are b4b or if the addition of
`target` causes the compiler to optimize differently.

4. Minor updates to
`test_prebuid/{test_blocked_data,test_chunked_data}`: fix wrong name of
subroutines in diagnostic error messages, set thread number and thread
counter to safe values when running over the entire domain.

scripts/mkcap.py

@@ -299,13 +299,14 @@ def print_def_intent(self, metadata):
  # It is an error for host model variables to have the optional attribute in the metadata
  if self.optional == 'T':
  error_message = "This routine should only be called for host model variables" + \
- " that cannot be optional, but got self.optional=T"
+ " that cannot have the optional metadata attribute, but got self.optional=T"
  raise Exception(error_message)
  # If the host variable is potentially unallocated, add optional and target to variable declaration
  elif not self.active == 'T':
  optional = ', optional, target'
  else:
- optional = ''
+ # Always declare as target variable so that locally defined pointers can point to it
+ optional = ', target'
  #
  if self.type in STANDARD_VARIABLE_TYPES:
  if self.kind:

scripts/mkstatic.py

@@ -211,15 +211,17 @@ def create_arguments_module_use_var_defs(variable_dictionary, metadata_define, t
  kind_var_standard_name = variable_dictionary[standard_name].kind
  if not kind_var_standard_name in local_kind_and_type_vars:
  if not kind_var_standard_name in metadata_define.keys():
- raise Exception("Kind {kind} not defined by host model".format(kind=kind_var_standard_name))
+ raise Exception("Kind {kind}, required by {std_name}, not defined by host model".format(
+ kind=kind_var_standard_name, std_name=standard_name))
  kind_var = metadata_define[kind_var_standard_name][0]
  module_use.append(kind_var.print_module_use())
  local_kind_and_type_vars.append(kind_var_standard_name)
  elif not variable_dictionary[standard_name].type in STANDARD_VARIABLE_TYPES:
  type_var_standard_name = variable_dictionary[standard_name].type
  if not type_var_standard_name in local_kind_and_type_vars:
  if not type_var_standard_name in metadata_define.keys():
- raise Exception("Type {type} not defined by host model".format(type=type_var_standard_name))
+ raise Exception("Type {type}, required by {std_name}, not defined by host model".format(
+ type=type_var_standard_name, std_name=standard_name))
  type_var = metadata_define[type_var_standard_name][0]
  module_use.append(type_var.print_module_use())
  local_kind_and_type_vars.append(type_var_standard_name)
@@ -1471,27 +1473,50 @@ def write(self, metadata_request, metadata_define, arguments, debug):
  if len(dimensions_target_name) < len(dim_substrings):
  raise Exception("THIS SHOULD NOT HAPPEN")
  dim_counter = 0
+ # We need two different dim strings for the following. The first,
+ # called 'dim_string' is used for the incoming variable (host model
+ # variable) and must contain all dimensions and indices. The second,
+ # called 'dim_string_allocate' is used for the allocation of temporary
+ # variables used for unit conversions etc. This 'dim_string_allocate'
+ # only contains the dimensions, not the indices of the target variable.
+ # Example: a scheme requests a variable foo that is a slice of a host
+ # model variable bar: foo(1:n) = bar(1:n,1). If a variable transformation
+ # is required, mkstatic creates a temporary variable tmpvar of rank 1.
+ # The allocation and assignment of this variable must then be
+ # allocate(tmpvar(1:n))
+ # tmpvar(1:n) = bar(1:n,1)
+ # Likewise, when scheme baz is called, the callstring must be
+ # call baz(...,foo=tmpvar(1:n),...)
+ # Hence the need for two different dim strings in the following code.
+ # See also https://github.com/NCAR/ccpp-framework/issues/598.
  dim_string = '('
+ dim_string_allocate = '('
  for dim in dimensions_target_name:
  if ':' in dim:
  dim_string += dim_substrings[dim_counter] + ','
+ dim_string_allocate += dim_substrings[dim_counter] + ','
  dim_counter += 1
  else:
  dim_string += dim + ','
+ # Don't add to dim_string_allocate!
  dim_string = dim_string.rstrip(',') + ')'
+ dim_string_allocate = dim_string_allocate.rstrip(',') + ')'
  # Consistency check to make sure all dimensions from metadata are 'used'
  if dim_counter < len(dim_substrings):
  raise Exception(f"Mismatch of derived dimensions from metadata {dim_substrings} " + \
  f"vs target local name {dimensions_target_name} for {var_standard_name} and " + \
  f"scheme {scheme_name} / phase {ccpp_stage}")
  else:
  dim_string = '({})'.format(','.join(dim_substrings))
+ dim_string_allocate = dim_string
  var_size_expected = '({})'.format('*'.join(array_size))
  else:
  if dimensions_target_name:
  dim_string = dim_string_target_name
  else:
  dim_string = ''
+ # A scalar variable doesn't get allocated, set to safe value
+ dim_string_allocate = ''
  var_size_expected = 1
 
  # To assist debugging efforts, check if arrays have the correct size (ignore scalars for now)
@@ -1717,7 +1742,7 @@ def write(self, metadata_request, metadata_define, arguments, debug):
  tmpvars[local_vars[var_standard_name]['name']] = tmpvar
  if tmpvar.rank:
  # Add allocate statement if the variable has a rank > 0 using the dimstring derived above
- actions_in += f' allocate({tmpvar.local_name}{dim_string})\n'
+ actions_in += f' allocate({tmpvar.local_name}{dim_string_allocate})\n'
  if var.actions['in']:
  # Add unit conversion before entering the subroutine
  actions_in += ' {t} = {c}{d}\n'.format(t=tmpvar.local_name,
@@ -1727,7 +1752,7 @@ def write(self, metadata_request, metadata_define, arguments, debug):
  # If the variable is conditionally allocated, assign pointer
  if not conditional == '.true.':
  actions_in += ' {p} => {t}{d}\n'.format(p=f"{tmpptr.local_name}_array({CCPP_INTERNAL_VARIABLES[CCPP_THREAD_NUMBER]})%p",
- t=tmpvar.local_name, d=dim_string)
+ t=tmpvar.local_name, d=dim_string_allocate)
  if var.actions['out']:
  # Add unit conversion after returning from the subroutine
  actions_out += ' {v}{d} = {c}\n'.format(v=tmpvar.target.replace(dim_string_target_name, ''),
@@ -1758,7 +1783,7 @@ def write(self, metadata_request, metadata_define, arguments, debug):
  # Add to argument list
  if conditional == '.true.':
  arg = '{local_name}={var_name}{dim_string},'.format(local_name=var.local_name,
- var_name=tmpvar.local_name.replace(dim_string_target_name, ''), dim_string=dim_string)
+ var_name=tmpvar.local_name.replace(dim_string_target_name, ''), dim_string=dim_string_allocate)
  else:
  arg = '{local_name}={ptr_name},'.format(local_name=var.local_name,
  ptr_name=f"{tmpptr.local_name}_array({CCPP_INTERNAL_VARIABLES[CCPP_THREAD_NUMBER]})%p")

test_prebuild/run_all_tests.sh

@@ -6,5 +6,6 @@ echo "" && echo "Running unit_tests " && cd unit_tests && ./run_tes
 echo "" && echo "Running test_opt_arg " && cd test_opt_arg && ./run_test.sh && cd ..
 echo "" && echo "Running test_blocked_data" && cd test_blocked_data && ./run_test.sh && cd ..
 echo "" && echo "Running test_chunked_data" && cd test_chunked_data && ./run_test.sh && cd ..
+echo "" && echo "Running test_unit_conv" && cd test_unit_conv && ./run_test.sh && cd ..
 
-echo "" && echo "Running test_track_variables" && pytest test_track_variables.py
+echo "" && echo "Running test_track_variables" && pytest test_track_variables.py

test_prebuild/test_blocked_data/main.F90

@@ -23,16 +23,21 @@ program test_blocked_data
  ! CCPP init step !
  !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 
- ! For physics running over the entire domain, block and thread
- ! number are not used; set to safe values
+ ! For physics running over the entire domain,
+ ! ccpp_thread_number and ccpp_chunk_number are
+ ! set to 1, indicating that arrays are to be sent
+ ! following their dimension specification in the
+ ! metadata (must match horizontal_dimension).
  ccpp_data_domain%blk_no = 1
  ccpp_data_domain%thrd_no = 1
+ ccpp_data_domain%thrd_cnt = 1
 
  ! Loop over all blocks and threads for ccpp_data_blocks
  do ib=1,nblks
  ! Assign the correct block numbers, only one thread
- ccpp_data_blocks(ib)%blk_no = ib
- ccpp_data_blocks(ib)%thrd_no = 1
+ ccpp_data_blocks(ib)%blk_no = ib
+ ccpp_data_blocks(ib)%thrd_no = 1
+ ccpp_data_blocks(ib)%thrd_cnt = 1
  end do
 
  do ib=1,size(blocked_data_instance)
@@ -85,7 +90,7 @@ program test_blocked_data
  cdata => ccpp_data_domain
  call ccpp_physics_timestep_finalize(cdata, suite_name=trim(ccpp_suite), ierr=ierr)
  if (ierr/=0) then
- write(error_unit,'(a)') "An error occurred in ccpp_physics_timestep_init:"
+ write(error_unit,'(a)') "An error occurred in ccpp_physics_timestep_finalize:"
  write(error_unit,'(a)') trim(cdata%errmsg)
  stop 1
  end if
@@ -97,7 +102,7 @@ program test_blocked_data
  cdata => ccpp_data_domain
  call ccpp_physics_finalize(cdata, suite_name=trim(ccpp_suite), ierr=ierr)
  if (ierr/=0) then
- write(error_unit,'(a)') "An error occurred in ccpp_physics_timestep_init:"
+ write(error_unit,'(a)') "An error occurred in ccpp_physics_finalize:"
  write(error_unit,'(a)') trim(cdata%errmsg)
  stop 1
  end if

test_prebuild/test_chunked_data/main.F90

@@ -25,11 +25,11 @@ program test_chunked_data
 
  ! For physics running over the entire domain,
  ! ccpp_thread_number and ccpp_chunk_number are
- ! set to 0, indicating that arrays are to be sent
+ ! set to 1, indicating that arrays are to be sent
  ! following their dimension specification in the
  ! metadata (must match horizontal_dimension).
- ccpp_data_domain%thrd_no = 0
- ccpp_data_domain%chunk_no = 0
+ ccpp_data_domain%thrd_no = 1
+ ccpp_data_domain%chunk_no = 1
  ccpp_data_domain%thrd_cnt = 1
 
  ! Loop over all chunks and threads for ccpp_data_chunks
@@ -88,7 +88,7 @@ program test_chunked_data
  cdata => ccpp_data_domain
  call ccpp_physics_timestep_finalize(cdata, suite_name=trim(ccpp_suite), ierr=ierr)
  if (ierr/=0) then
- write(error_unit,'(a)') "An error occurred in ccpp_physics_timestep_init:"
+ write(error_unit,'(a)') "An error occurred in ccpp_physics_timestep_finalize:"
  write(error_unit,'(a)') trim(cdata%errmsg)
  stop 1
  end if
@@ -100,7 +100,7 @@ program test_chunked_data
  cdata => ccpp_data_domain
  call ccpp_physics_finalize(cdata, suite_name=trim(ccpp_suite), ierr=ierr)
  if (ierr/=0) then
- write(error_unit,'(a)') "An error occurred in ccpp_physics_timestep_init:"
+ write(error_unit,'(a)') "An error occurred in ccpp_physics_finalize:"
  write(error_unit,'(a)') trim(cdata%errmsg)
  stop 1
  end if

test_prebuild/test_unit_conv/CMakeLists.txt

@@ -0,0 +1,98 @@
+#------------------------------------------------------------------------------
+cmake_minimum_required(VERSION 3.10)
+
+project(ccpp_unit_conv
+ VERSION 1.0.0
+ LANGUAGES Fortran)
+
+#------------------------------------------------------------------------------
+# Request a static build
+option(BUILD_SHARED_LIBS "Build a shared library" OFF)
+
+#------------------------------------------------------------------------------
+# Set MPI flags for C/C++/Fortran with MPI F08 interface
+find_package(MPI REQUIRED Fortran)
+if(NOT MPI_Fortran_HAVE_F08_MODULE)
+ message(FATAL_ERROR "MPI implementation does not support the Fortran 2008 mpi_f08 interface")
+endif()
+
+#------------------------------------------------------------------------------
+# Set the sources: physics type definitions
+set(TYPEDEFS $ENV{CCPP_TYPEDEFS})
+if(TYPEDEFS)
+ message(STATUS "Got CCPP TYPEDEFS from environment variable: ${TYPEDEFS}")
+else(TYPEDEFS)
+ include(${CMAKE_CURRENT_BINARY_DIR}/CCPP_TYPEDEFS.cmake)
+ message(STATUS "Got CCPP TYPEDEFS from cmakefile include file: ${TYPEDEFS}")
+endif(TYPEDEFS)
+
+# Generate list of Fortran modules from the CCPP type
+# definitions that need need to be installed
+foreach(typedef_module ${TYPEDEFS})
+ list(APPEND MODULES_F90 ${CMAKE_CURRENT_BINARY_DIR}/${typedef_module})
+endforeach()
+
+#------------------------------------------------------------------------------
+# Set the sources: physics schemes
+set(SCHEMES $ENV{CCPP_SCHEMES})
+if(SCHEMES)
+ message(STATUS "Got CCPP SCHEMES from environment variable: ${SCHEMES}")
+else(SCHEMES)
+ include(${CMAKE_CURRENT_BINARY_DIR}/CCPP_SCHEMES.cmake)
+ message(STATUS "Got CCPP SCHEMES from cmakefile include file: ${SCHEMES}")
+endif(SCHEMES)
+
+# Set the sources: physics scheme caps
+set(CAPS $ENV{CCPP_CAPS})
+if(CAPS)
+ message(STATUS "Got CCPP CAPS from environment variable: ${CAPS}")
+else(CAPS)
+ include(${CMAKE_CURRENT_BINARY_DIR}/CCPP_CAPS.cmake)
+ message(STATUS "Got CCPP CAPS from cmakefile include file: ${CAPS}")
+endif(CAPS)
+
+# Set the sources: physics scheme caps
+set(API $ENV{CCPP_API})
+if(API)
+ message(STATUS "Got CCPP API from environment variable: ${API}")
+else(API)
+ include(${CMAKE_CURRENT_BINARY_DIR}/CCPP_API.cmake)
+ message(STATUS "Got CCPP API from cmakefile include file: ${API}")
+endif(API)
+
+set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -O0 -fno-unsafe-math-optimizations -frounding-math -fsignaling-nans -ffpe-trap=invalid,zero,overflow -fbounds-check -ggdb -fbacktrace -ffree-line-length-none")
+
+#------------------------------------------------------------------------------
+add_library(ccpp_unit_conv STATIC ${SCHEMES} ${CAPS} ${API})
+target_link_libraries(ccpp_unit_conv PRIVATE MPI::MPI_Fortran)
+# Generate list of Fortran modules from defined sources
+foreach(source_f90 ${CAPS} ${API})
+ get_filename_component(tmp_source_f90 ${source_f90} NAME)
+ string(REGEX REPLACE ".F90" ".mod" tmp_module_f90 ${tmp_source_f90})
+ string(TOLOWER ${tmp_module_f90} module_f90)
+ list(APPEND MODULES_F90 ${CMAKE_CURRENT_BINARY_DIR}/${module_f90})
+endforeach()
+
+set_target_properties(ccpp_unit_conv PROPERTIES VERSION ${PROJECT_VERSION}
+ SOVERSION ${PROJECT_VERSION_MAJOR})
+
+add_executable(test_unit_conv.x main.F90)
+add_dependencies(test_unit_conv.x ccpp_unit_conv)
+target_link_libraries(test_unit_conv.x ccpp_unit_conv)
+set_target_properties(test_unit_conv.x PROPERTIES LINKER_LANGUAGE Fortran)
+
+# Define where to install the library
+install(TARGETS ccpp_unit_conv
+ EXPORT ccpp_unit_conv-targets
+ ARCHIVE DESTINATION lib
+ LIBRARY DESTINATION lib
+ RUNTIME DESTINATION lib
+)
+# Export our configuration
+install(EXPORT ccpp_unit_conv-targets
+ FILE ccpp_unit_conv-config.cmake
+ DESTINATION lib/cmake
+)
+# Define where to install the C headers and Fortran modules
+#install(FILES ${HEADERS_C} DESTINATION include)
+install(FILES ${MODULES_F90} DESTINATION include)

test_prebuild/test_unit_conv/README.md

@@ -0,0 +1,16 @@
+# How to build the unit conv test
+
+1. Set compiler environment as appropriate for your system
+2. Run the following commands:
+```
+cd test_prebuild/test_unit_conv/
+rm -fr build
+mkdir build
+../../scripts/ccpp_prebuild.py --config=ccpp_prebuild_config.py --builddir=build
+cd build
+cmake .. 2>&1 | tee log.cmake
+make 2>&1 | tee log.make
+./test_unit_conv.x
+# On systems where linking against the MPI library requires a parallel launcher,
+# use 'mpirun -np 1 ./test_unit_conv.x' or 'srun -n 1 ./test_unit_conv.x' etc.
+```

test_prebuild/test_unit_conv/ccpp_kinds.F90

@@ -0,0 +1,13 @@
+module ccpp_kinds
+
+!! \section arg_table_ccpp_kinds
+!! \htmlinclude ccpp_kinds.html
+!!
+
+ use iso_fortran_env, only: real64
+
+ implicit none
+
+ integer, parameter :: kind_phys = real64
+
+end module ccpp_kinds

test_prebuild/test_unit_conv/ccpp_kinds.meta

@@ -0,0 +1,15 @@
+[ccpp-table-properties]
+ name = ccpp_kinds
+ type = module
+ dependencies = 
+
+########################################################################
+[ccpp-arg-table]
+ name = ccpp_kinds
+ type = module
+[kind_phys]
+ standard_name = kind_phys
+ long_name = definition of kind_phys
+ units = none
+ dimensions = ()
+ type = integer