Bug fix for unit conversion error in ccpp_prebuild.py (variables that are slices of a n+1 dimensional array have wrong allocation)

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

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 chunked data test
+
+1. Set compiler environment as appropriate for your system
+2. Run the following commands:
+```
+cd test_prebuild/test_chunked_data/
+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_chunked_data.x
+# On systems where linking against the MPI library requires a parallel launcher,
+# use 'mpirun -np 1 ./test_chunked_data.x' or 'srun -n 1 ./test_chunked_data.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