From b7d55fd07fa35c5de9b2292596d30784a800b276 Mon Sep 17 00:00:00 2001 From: Dom Heinzeller Date: Tue, 22 Oct 2024 16:29:25 -0600 Subject: [PATCH 1/3] 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 https://github.com/NCAR/ccpp-framework/issues/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 | 5 +- scripts/mkstatic.py | 35 ++++++- test_prebuild/run_all_tests.sh | 3 +- test_prebuild/test_blocked_data/main.F90 | 17 ++-- test_prebuild/test_chunked_data/main.F90 | 10 +- test_prebuild/test_unit_conv/CMakeLists.txt | 98 +++++++++++++++++++ test_prebuild/test_unit_conv/README.md | 16 +++ test_prebuild/test_unit_conv/ccpp_kinds.F90 | 13 +++ test_prebuild/test_unit_conv/ccpp_kinds.meta | 15 +++ .../test_unit_conv/ccpp_prebuild_config.py | 82 ++++++++++++++++ test_prebuild/test_unit_conv/data.F90 | 22 +++++ test_prebuild/test_unit_conv/data.meta | 53 ++++++++++ test_prebuild/test_unit_conv/main.F90 | 92 +++++++++++++++++ test_prebuild/test_unit_conv/run_test.sh | 13 +++ .../test_unit_conv/suite_unit_conv_suite.xml | 11 +++ .../test_unit_conv/unit_conv_scheme_1.F90 | 55 +++++++++++ .../test_unit_conv/unit_conv_scheme_1.meta | 41 ++++++++ .../test_unit_conv/unit_conv_scheme_2.F90 | 55 +++++++++++ .../test_unit_conv/unit_conv_scheme_2.meta | 41 ++++++++ 19 files changed, 658 insertions(+), 19 deletions(-) create mode 100644 test_prebuild/test_unit_conv/CMakeLists.txt create mode 100644 test_prebuild/test_unit_conv/README.md create mode 100644 test_prebuild/test_unit_conv/ccpp_kinds.F90 create mode 100644 test_prebuild/test_unit_conv/ccpp_kinds.meta create mode 100755 test_prebuild/test_unit_conv/ccpp_prebuild_config.py create mode 100644 test_prebuild/test_unit_conv/data.F90 create mode 100644 test_prebuild/test_unit_conv/data.meta create mode 100644 test_prebuild/test_unit_conv/main.F90 create mode 100755 test_prebuild/test_unit_conv/run_test.sh create mode 100644 test_prebuild/test_unit_conv/suite_unit_conv_suite.xml create mode 100644 test_prebuild/test_unit_conv/unit_conv_scheme_1.F90 create mode 100644 test_prebuild/test_unit_conv/unit_conv_scheme_1.meta create mode 100644 test_prebuild/test_unit_conv/unit_conv_scheme_2.F90 create mode 100644 test_prebuild/test_unit_conv/unit_conv_scheme_2.meta diff --git a/scripts/mkcap.py b/scripts/mkcap.py index 8a8ca7d9..ccf937d3 100755 --- a/scripts/mkcap.py +++ b/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: diff --git a/scripts/mkstatic.py b/scripts/mkstatic.py index 4f00b9b9..c24fd8bb 100755 --- a/scripts/mkstatic.py +++ b/scripts/mkstatic.py @@ -211,7 +211,8 @@ 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) @@ -219,7 +220,8 @@ def create_arguments_module_use_var_defs(variable_dictionary, metadata_define, t 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,14 +1473,34 @@ 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} " + \ @@ -1486,12 +1508,15 @@ def write(self, metadata_request, metadata_define, arguments, debug): 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") diff --git a/test_prebuild/run_all_tests.sh b/test_prebuild/run_all_tests.sh index 2e199500..2de74906 100755 --- a/test_prebuild/run_all_tests.sh +++ b/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 \ No newline at end of file +echo "" && echo "Running test_track_variables" && pytest test_track_variables.py diff --git a/test_prebuild/test_blocked_data/main.F90 b/test_prebuild/test_blocked_data/main.F90 index cc57f618..4711b3c9 100644 --- a/test_prebuild/test_blocked_data/main.F90 +++ b/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 diff --git a/test_prebuild/test_chunked_data/main.F90 b/test_prebuild/test_chunked_data/main.F90 index 8864aa80..a1af449b 100644 --- a/test_prebuild/test_chunked_data/main.F90 +++ b/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 diff --git a/test_prebuild/test_unit_conv/CMakeLists.txt b/test_prebuild/test_unit_conv/CMakeLists.txt new file mode 100644 index 00000000..97c1730c --- /dev/null +++ b/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) diff --git a/test_prebuild/test_unit_conv/README.md b/test_prebuild/test_unit_conv/README.md new file mode 100644 index 00000000..17ca1c58 --- /dev/null +++ b/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. +``` diff --git a/test_prebuild/test_unit_conv/ccpp_kinds.F90 b/test_prebuild/test_unit_conv/ccpp_kinds.F90 new file mode 100644 index 00000000..cf6bfeaf --- /dev/null +++ b/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 diff --git a/test_prebuild/test_unit_conv/ccpp_kinds.meta b/test_prebuild/test_unit_conv/ccpp_kinds.meta new file mode 100644 index 00000000..0e95702e --- /dev/null +++ b/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 diff --git a/test_prebuild/test_unit_conv/ccpp_prebuild_config.py b/test_prebuild/test_unit_conv/ccpp_prebuild_config.py new file mode 100755 index 00000000..3ee45942 --- /dev/null +++ b/test_prebuild/test_unit_conv/ccpp_prebuild_config.py @@ -0,0 +1,82 @@ +#!/usr/bin/env python + +# CCPP prebuild config for GFDL Finite-Volume Cubed-Sphere Model (FV3) + +############################################################################### +# Definitions # +############################################################################### + +HOST_MODEL_IDENTIFIER = "FV3" + +# Add all files with metadata tables on the host model side and in CCPP, +# relative to basedir = top-level directory of host model. This includes +# kind and type definitions used in CCPP physics. Also add any internal +# dependencies of these files to the list. +VARIABLE_DEFINITION_FILES = [ + # actual variable definition files + '../../src/ccpp_types.F90', + 'ccpp_kinds.F90', + 'data.F90', + ] + +TYPEDEFS_NEW_METADATA = { + 'ccpp_types' : { + 'ccpp_t' : 'cdata', + 'ccpp_types' : '', + }, + 'data' : { + 'data' : '', + }, + } + +# Add all physics scheme files relative to basedir +SCHEME_FILES = [ + 'unit_conv_scheme_1.F90', + 'unit_conv_scheme_2.F90', + ] + +# Default build dir, relative to current working directory, +# if not specified as command-line argument +DEFAULT_BUILD_DIR = 'build' + +# Auto-generated makefile/cmakefile snippets that contain all type definitions +TYPEDEFS_MAKEFILE = '{build_dir}/CCPP_TYPEDEFS.mk' +TYPEDEFS_CMAKEFILE = '{build_dir}/CCPP_TYPEDEFS.cmake' +TYPEDEFS_SOURCEFILE = '{build_dir}/CCPP_TYPEDEFS.sh' + +# Auto-generated makefile/cmakefile snippets that contain all schemes +SCHEMES_MAKEFILE = '{build_dir}/CCPP_SCHEMES.mk' +SCHEMES_CMAKEFILE = '{build_dir}/CCPP_SCHEMES.cmake' +SCHEMES_SOURCEFILE = '{build_dir}/CCPP_SCHEMES.sh' + +# Auto-generated makefile/cmakefile snippets that contain all caps +CAPS_MAKEFILE = '{build_dir}/CCPP_CAPS.mk' +CAPS_CMAKEFILE = '{build_dir}/CCPP_CAPS.cmake' +CAPS_SOURCEFILE = '{build_dir}/CCPP_CAPS.sh' + +# Directory where to put all auto-generated physics caps +CAPS_DIR = '{build_dir}' + +# Directory where the suite definition files are stored +SUITES_DIR = '.' + +# Optional arguments - only required for schemes that use +# optional arguments. ccpp_prebuild.py will throw an exception +# if it encounters a scheme subroutine with optional arguments +# if no entry is made here. Possible values are: 'all', 'none', +# or a list of standard_names: [ 'var1', 'var3' ]. +OPTIONAL_ARGUMENTS = {} + +# Directory where to write static API to +STATIC_API_DIR = '{build_dir}' +STATIC_API_CMAKEFILE = '{build_dir}/CCPP_API.cmake' +STATIC_API_SOURCEFILE = '{build_dir}/CCPP_API.sh' + +# Directory for writing HTML pages generated from metadata files +METADATA_HTML_OUTPUT_DIR = '{build_dir}' + +# HTML document containing the model-defined CCPP variables +HTML_VARTABLE_FILE = '{build_dir}/CCPP_VARIABLES_UNIT_CONV.html' + +# LaTeX document containing the provided vs requested CCPP variables +LATEX_VARTABLE_FILE = '{build_dir}/CCPP_VARIABLES_UNIT_CONV.tex' diff --git a/test_prebuild/test_unit_conv/data.F90 b/test_prebuild/test_unit_conv/data.F90 new file mode 100644 index 00000000..f53c107e --- /dev/null +++ b/test_prebuild/test_unit_conv/data.F90 @@ -0,0 +1,22 @@ +module data + +!! \section arg_table_data Argument Table +!! \htmlinclude data.html +!! + use ccpp_kinds, only : kind_phys + use ccpp_types, only: ccpp_t + + implicit none + + private + + public ncols, nspecies + public cdata, data_array, opt_array_flag + + integer, parameter :: ncols = 4 + integer, parameter :: nspecies = 2 + type(ccpp_t), target :: cdata + real(kind_phys), dimension(1:ncols,1:nspecies) :: data_array + logical :: opt_array_flag + +end module data diff --git a/test_prebuild/test_unit_conv/data.meta b/test_prebuild/test_unit_conv/data.meta new file mode 100644 index 00000000..895cd6c2 --- /dev/null +++ b/test_prebuild/test_unit_conv/data.meta @@ -0,0 +1,53 @@ +[ccpp-table-properties] + name = data + type = module + dependencies = +[ccpp-arg-table] + name = data + type = module +[cdata] + standard_name = ccpp_t_instance + long_name = instance of derived data type ccpp_t + units = DDT + dimensions = () + type = ccpp_t +[ncols] + standard_name = horizontal_loop_extent + long_name = horizontal loop extent + units = count + dimensions = () + type = integer +[nspecies] + standard_name = number_of_species + long_name = number of species in data array + units = count + dimensions = () + type = integer +[data_array] + standard_name = data_array_all_species + long_name = data array in module + units = m + dimensions = (horizontal_loop_extent, number_of_species) + type = real + kind = kind_phys +[data_array(:,2)] + standard_name = data_array + long_name = data array in module + units = m + dimensions = (horizontal_loop_extent) + type = real + kind = kind_phys +[opt_array_flag] + standard_name = flag_for_opt_array + long_name = flag for passing optional data array + units = 1 + dimensions = () + type = logical +[data_array(:,1)] + standard_name = data_array_opt + long_name = optional data array in km + units = m + dimensions = (horizontal_loop_extent) + type = real + kind = kind_phys + active = (flag_for_opt_array) diff --git a/test_prebuild/test_unit_conv/main.F90 b/test_prebuild/test_unit_conv/main.F90 new file mode 100644 index 00000000..3f7ee103 --- /dev/null +++ b/test_prebuild/test_unit_conv/main.F90 @@ -0,0 +1,92 @@ +program test_unit_conv + + use, intrinsic :: iso_fortran_env, only: error_unit + + use ccpp_types, only: ccpp_t + use data, only: ncols, nspecies + use data, only: cdata, data_array, opt_array_flag + + use ccpp_static_api, only: ccpp_physics_init, & + ccpp_physics_timestep_init, & + ccpp_physics_run, & + ccpp_physics_timestep_finalize, & + ccpp_physics_finalize + + implicit none + + character(len=*), parameter :: ccpp_suite = 'unit_conv_suite' + integer :: ierr + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + ! CCPP init step ! + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + + ! 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). + cdata%thrd_no = 1 + cdata%chunk_no = 1 + cdata%thrd_cnt = 1 + + data_array = 1.0_8 + opt_array_flag = .true. + + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + ! CCPP physics init step ! + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + + call ccpp_physics_init(cdata, suite_name=trim(ccpp_suite), ierr=ierr) + if (ierr/=0) then + write(error_unit,'(a)') "An error occurred in ccpp_physics_init:" + write(error_unit,'(a)') trim(cdata%errmsg) + stop 1 + end if + + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + ! CCPP physics timestep init step ! + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + + call ccpp_physics_timestep_init(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)') trim(cdata%errmsg) + stop 1 + end if + + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + ! CCPP physics run step ! + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + + call ccpp_physics_run(cdata, suite_name=trim(ccpp_suite), ierr=ierr) + if (ierr/=0) then + write(error_unit,'(a)') "An error occurred in ccpp_physics_run:" + write(error_unit,'(a)') trim(cdata%errmsg) + stop 1 + end if + + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + ! CCPP physics timestep finalize step ! + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + + 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_finalize:" + write(error_unit,'(a)') trim(cdata%errmsg) + stop 1 + end if + + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + ! CCPP physics finalize step ! + !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + + 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)') trim(cdata%errmsg) + stop 1 + end if + +contains + +end program test_unit_conv diff --git a/test_prebuild/test_unit_conv/run_test.sh b/test_prebuild/test_unit_conv/run_test.sh new file mode 100755 index 00000000..ab7e8c31 --- /dev/null +++ b/test_prebuild/test_unit_conv/run_test.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash + +set -e + +rm -fr build +mkdir build +../../scripts/ccpp_prebuild.py --verbose --debug --config=ccpp_prebuild_config.py --builddir=build +cd build +cmake .. 2>&1 | tee log.cmake +make VERBOSE=1 -j1 2>&1 | tee log.make +./test_unit_conv.x +cd .. +rm -fr build diff --git a/test_prebuild/test_unit_conv/suite_unit_conv_suite.xml b/test_prebuild/test_unit_conv/suite_unit_conv_suite.xml new file mode 100644 index 00000000..68d90109 --- /dev/null +++ b/test_prebuild/test_unit_conv/suite_unit_conv_suite.xml @@ -0,0 +1,11 @@ + + + + + + unit_conv_scheme_1 + unit_conv_scheme_2 + unit_conv_scheme_1 + + + diff --git a/test_prebuild/test_unit_conv/unit_conv_scheme_1.F90 b/test_prebuild/test_unit_conv/unit_conv_scheme_1.F90 new file mode 100644 index 00000000..d9488789 --- /dev/null +++ b/test_prebuild/test_unit_conv/unit_conv_scheme_1.F90 @@ -0,0 +1,55 @@ +!>\file unit_conv_scheme_1.F90 +!! This file contains a unit_conv_scheme_1 CCPP scheme that does nothing +!! except requesting the minimum, mandatory variables. + +module unit_conv_scheme_1 + + use, intrinsic :: iso_fortran_env, only: error_unit + use ccpp_kinds, only : kind_phys + implicit none + + private + public :: unit_conv_scheme_1_run + + !! This is for unit testing only + real(kind_phys), parameter :: target_value = 1.0_kind_phys + + contains + +!! \section arg_table_unit_conv_scheme_1_run Argument Table +!! \htmlinclude unit_conv_scheme_1_run.html +!! + subroutine unit_conv_scheme_1_run(data_array, data_array_opt, errmsg, errflg) + character(len=*), intent(out) :: errmsg + integer, intent(out) :: errflg + real(kind_phys), intent(inout) :: data_array(:) + real(kind_phys), intent(inout), optional :: data_array_opt(:) + + ! Initialize CCPP error handling variables + errmsg = '' + errflg = 0 + ! Check values in data array + write(error_unit,'(a,e12.4)') 'In unit_conv_scheme_1_run: checking min/max values of data array to be approximately ', target_value + if (minval(data_array)<0.99*target_value .or. maxval(data_array)>1.01*target_value) then + write(errmsg,'(3(a,e12.4),a)') "Error in unit_conv_scheme_1_run, expected values of approximately ", & + target_value, " but got [ ", minval(data_array), " : ", maxval(data_array), " ]" + errflg = 1 + return + end if + ! Check for presence of optional data array, then check its values + write(error_unit,'(a)') 'In unit_conv_scheme_1_run: checking for presence of optional data array' + if (.not. present(data_array_opt)) then + write(error_unit,'(a)') 'Error in unit_conv_scheme_1_run, optional data array expected but not present' + errflg = 1 + return + endif + write(error_unit,'(a,e12.4)') 'In unit_conv_scheme_1_run: checking min/max values of optional data array to be approximately ', target_value + if (minval(data_array_opt)<0.99*target_value .or. maxval(data_array_opt)>1.01*target_value) then + write(errmsg,'(3(a,e12.4),a)') 'Error in unit_conv_scheme_1_run, expected values of approximately ', & + target_value, ' but got [ ', minval(data_array_opt), ' : ', maxval(data_array_opt), ' ]' + errflg = 1 + return + end if + end subroutine unit_conv_scheme_1_run + +end module unit_conv_scheme_1 diff --git a/test_prebuild/test_unit_conv/unit_conv_scheme_1.meta b/test_prebuild/test_unit_conv/unit_conv_scheme_1.meta new file mode 100644 index 00000000..91f22142 --- /dev/null +++ b/test_prebuild/test_unit_conv/unit_conv_scheme_1.meta @@ -0,0 +1,41 @@ +[ccpp-table-properties] + name = unit_conv_scheme_1 + type = scheme + dependencies = + +######################################################################## +[ccpp-arg-table] + name = unit_conv_scheme_1_run + type = scheme +[errmsg] + standard_name = ccpp_error_message + long_name = error message for error handling in CCPP + units = none + dimensions = () + type = character + kind = len=* + intent = out +[errflg] + standard_name = ccpp_error_code + long_name = error code for error handling in CCPP + units = 1 + dimensions = () + type = integer + intent = out +[data_array] + standard_name = data_array + long_name = data array in m + units = m + dimensions = (horizontal_loop_extent) + type = real + kind = kind_phys + intent = inout +[data_array_opt] + standard_name = data_array_opt + long_name = optional data array in m + units = m + dimensions = (horizontal_loop_extent) + type = real + kind = kind_phys + intent = inout + optional = True diff --git a/test_prebuild/test_unit_conv/unit_conv_scheme_2.F90 b/test_prebuild/test_unit_conv/unit_conv_scheme_2.F90 new file mode 100644 index 00000000..6b64402c --- /dev/null +++ b/test_prebuild/test_unit_conv/unit_conv_scheme_2.F90 @@ -0,0 +1,55 @@ +!>\file unit_conv_scheme_2.F90 +!! This file contains a unit_conv_scheme_2 CCPP scheme that does nothing +!! except requesting the minimum, mandatory variables. + +module unit_conv_scheme_2 + + use, intrinsic :: iso_fortran_env, only: error_unit + use ccpp_kinds, only : kind_phys + implicit none + + private + public :: unit_conv_scheme_2_run + + !! This is for unit testing only + real(kind_phys), parameter :: target_value = 1.0E-3_kind_phys + + contains + +!! \section arg_table_unit_conv_scheme_2_run Argument Table +!! \htmlinclude unit_conv_scheme_2_run.html +!! + subroutine unit_conv_scheme_2_run(data_array, data_array_opt, errmsg, errflg) + character(len=*), intent(out) :: errmsg + integer, intent(out) :: errflg + real(kind_phys), intent(inout) :: data_array(:) + real(kind_phys), intent(inout), optional :: data_array_opt(:) + + ! Initialize CCPP error handling variables + errmsg = '' + errflg = 0 + ! Check values in data array + write(error_unit,'(a,e12.4)') 'In unit_conv_scheme_2_run: checking min/max values of data array to be approximately ', target_value + if (minval(data_array)<0.99*target_value .or. maxval(data_array)>1.01*target_value) then + write(errmsg,'(3(a,e12.4),a)') 'Error in unit_conv_scheme_2_run, expected values of approximately ', & + target_value, ' but got [ ', minval(data_array), ' : ', maxval(data_array), ' ]' + errflg = 1 + return + end if + ! Check for presence of optional data array, then check its values + write(error_unit,'(a)') 'In unit_conv_scheme_2_run: checking for presence of optional data array' + if (.not. present(data_array_opt)) then + write(error_unit,'(a)') 'Error in unit_conv_scheme_2_run, optional data array expected but not present' + errflg = 1 + return + endif + write(error_unit,'(a,e12.4)') 'In unit_conv_scheme_2_run: checking min/max values of optional data array to be approximately ', target_value + if (minval(data_array_opt)<0.99*target_value .or. maxval(data_array_opt)>1.01*target_value) then + write(errmsg,'(3(a,e12.4),a)') 'Error in unit_conv_scheme_2_run, expected values of approximately ', & + target_value, ' but got [ ', minval(data_array_opt), ' : ', maxval(data_array_opt), ' ]' + errflg = 1 + return + end if + end subroutine unit_conv_scheme_2_run + +end module unit_conv_scheme_2 diff --git a/test_prebuild/test_unit_conv/unit_conv_scheme_2.meta b/test_prebuild/test_unit_conv/unit_conv_scheme_2.meta new file mode 100644 index 00000000..534e3abe --- /dev/null +++ b/test_prebuild/test_unit_conv/unit_conv_scheme_2.meta @@ -0,0 +1,41 @@ +[ccpp-table-properties] + name = unit_conv_scheme_2 + type = scheme + dependencies = + +######################################################################## +[ccpp-arg-table] + name = unit_conv_scheme_2_run + type = scheme +[errmsg] + standard_name = ccpp_error_message + long_name = error message for error handling in CCPP + units = none + dimensions = () + type = character + kind = len=* + intent = out +[errflg] + standard_name = ccpp_error_code + long_name = error code for error handling in CCPP + units = 1 + dimensions = () + type = integer + intent = out +[data_array] + standard_name = data_array + long_name = data array in km + units = km + dimensions = (horizontal_loop_extent) + type = real + kind = kind_phys + intent = inout +[data_array_opt] + standard_name = data_array_opt + long_name = optional data array in km + units = km + dimensions = (horizontal_loop_extent) + type = real + kind = kind_phys + intent = inout + optional = True From 9e1c3abe1048c0f18c53fdbb7113bc56a129bdf5 Mon Sep 17 00:00:00 2001 From: Dom Heinzeller Date: Thu, 21 Nov 2024 15:21:58 -0500 Subject: [PATCH 2/3] Update CMakeLists.txt: set minimum cmake version to 3.18 (#611) Resolves https://github.com/NCAR/ccpp-framework/issues/610 --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 90634bee..9d03783d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,4 +1,4 @@ -cmake_minimum_required(VERSION 3.10) +cmake_minimum_required(VERSION 3.18) project(ccpp_framework VERSION 5.0.0 From 0345a6630c68f7270b22b69b76b3908a3aa67634 Mon Sep 17 00:00:00 2001 From: Dom Heinzeller Date: Mon, 9 Dec 2024 15:24:50 -0700 Subject: [PATCH 3/3] Merge two workarounds for ccpp-prebuild for handling optional arguments (#617) **This PR affects ccpp-prebuild only. It can be merged into develop (or main), but it must come to main as soon as possible for use in UFS/SCM.** This PR adds workarounds for handling optional arguments the right way (finally!) in `scripts/ccpp_prebuild.py` and `scripts/mkcap.py`. This update is already in use in NEPTUNE and is required for @dustinswales' work to update/revert the optional arguments in ccpp-physics in the UFS and the SCM. The workaround for `ccpp-prebuild` allows us to treat only those arguments as optional that are truly optional for a CCPP scheme. In the past, any argument that was conditionally allocated by any of the host models had to be declared as optional, even if it was required by the physics. User interface changes?: Yes and No. This can be merged without making any changes (it won't break the previous functionality where any conditionally allocated variable had to be declared as optional in the physics). But it will allow to declare many CCPP physics variables as non-optional if they aren't really optional. This finally resolves https://github.com/NCAR/ccpp-framework/issues/566 (by making ccpp-prebuild behave the same as capgen, which is the correct way to handle optional arguments). Testing: test removed: none unit tests: all pass system tests: all pass manual testing: implemented and tested thoroughly in NEPTUNE --- scripts/ccpp_prebuild.py | 7 +++++-- scripts/mkcap.py | 16 ++++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/scripts/ccpp_prebuild.py b/scripts/ccpp_prebuild.py index 3cb4ee19..b8829b6f 100755 --- a/scripts/ccpp_prebuild.py +++ b/scripts/ccpp_prebuild.py @@ -490,9 +490,12 @@ def compare_metadata(metadata_define, metadata_request): if not metadata_define[var_name][0].active == 'T': for var in metadata_request[var_name]: if var.optional == 'F': - logging.error("Conditionally allocated host-model variable {0} is not optional in {1}".format( + # DH 20241022 - change logging.error to logging.warn, because it is known + # that this strict check is not correct and will be reverted soon + #logging.error( + logging.warn("Conditionally allocated host-model variable {0} is not optional in {1}".format( var_name, var.container)) - success = False + #success = False # TEMPORARY CHECK - IF THE VARIABLE IS ALWAYS ALLOCATED, THE SCHEME VARIABLE SHOULDN'T BE OPTIONAL else: for var in metadata_request[var_name]: diff --git a/scripts/mkcap.py b/scripts/mkcap.py index ccf937d3..69c2292a 100755 --- a/scripts/mkcap.py +++ b/scripts/mkcap.py @@ -356,12 +356,16 @@ def print_def_local(self, metadata): str = "type({s.type}), pointer :: p => null()" return str.format(s=self) else: - # If the host variable is potentially unallocated, the active attribute is - # also set accordingly for the local variable; add target to variable declaration - if self.optional == 'T': - target = ', target' - else: - target = '' + # DH* 20241022 WORKAROUND TO ACCOUNT FOR MISSING UPDATES TO CCPP PHYSICS + # W.R.T. DECLARING OPTIONAL VARIABLES IN METADATA AND CODE. ALWAYS USE TARGET + ## If the host variable is potentially unallocated, the active attribute is + ## also set accordingly for the local variable; add target to variable declaration + #if self.optional == 'T': + # target = ', target' + #else: + # target = '' + target = ', target' + # *DH if self.type in STANDARD_VARIABLE_TYPES: if self.kind: if self.rank: