Merge branch 'match_file_type_on_read_v2'

Fix reading netcdf variable into a different data type

Main change is to fix the reading of a variable that is one type on a
NetCDF file but a different data type in memory. Previously, this could
lead to memory corruption (not just of the variable in question, but
other variables as well). This was not causing any problems in
out-of-the-box configurations, but it sometimes tripped up some
developers when they added new fields on datasets.

In addition, this adds new self-test code to test ncdio_pio. This module
is hard to get under unit test, but we want to have some tests of it, to
pick up problems like the one fixed in this tag. So as a compromise
solution for now, I have introduced unit test-like tests that are built
into a standard build of CTSM and run in a standard run if a given
namelist flag is set. A better long-term solution would be to integrate
these tests into the pFUnit-based unit testing framework, but that would
take some work. I have added tests of the new code, and a bit of testing
of other code in ncdio_pio, but there's still a lot in ncdio_pio that is
not tested. I figure we can gradually add tests as we make changes to
ncdio_pio.

Finally, an unrelated change to the unit test build: files generated by
genf90 now appear in the unit test build directory rather than the
source tree.

Resolves #1091 (Memory corruption when reading a netcdf
variable into a variable of a different type, at least with PIO1)

Resolves #1188 (In unit test build: files generated by
genf90 should go in build dir rather than source dir)

.gitignore

@@ -47,16 +47,6 @@ CMakeFiles/
 # unit testing directories
 /src/unit_tests.*
 
-# files generated by the unit test build
-/src/dyn_subgrid/dynVarMod.F90
-/src/dyn_subgrid/dynVarTimeInterpMod.F90
-/src/dyn_subgrid/dynVarTimeUninterpMod.F90
-/src/utils/array_utils.F90
-/src/unit_test_stubs/utils/restUtilMod_stub.F90
-/src/unit_test_stubs/main/ncdio_pio_fake.F90
-/src/unit_test_stubs/main/ncdio_var.F90
-/src/unit_test_shr/unittestArrayMod.F90
-
 # cime_config
 buildnmlc
 

bld/CLMBuildNamelist.pm

@@ -1496,6 +1496,7 @@ sub process_namelist_inline_logic {
   setup_logic_supplemental_nitrogen($opts, $nl_flags, $definition, $defaults, $nl);
   setup_logic_snowpack($opts,  $nl_flags, $definition, $defaults, $nl);
   setup_logic_fates($opts,  $nl_flags, $definition, $defaults, $nl);
+  setup_logic_misc($opts, $nl_flags, $definition, $defaults, $nl);
 
   #########################################
   # namelist group: atm2lnd_inparm
@@ -3883,6 +3884,17 @@ sub setup_logic_fates {
 
 #-------------------------------------------------------------------------------
 
+sub setup_logic_misc {
+   #
+   # Set some misc options
+   #
+   my ($opts, $nl_flags, $definition, $defaults, $nl) = @_;
+
+   add_default($opts, $nl_flags->{'inputdata_rootdir'}, $definition, $defaults, $nl, 'for_testing_run_ncdiopio_tests');
+}
+
+#-------------------------------------------------------------------------------
+
 sub write_output_files {
   my ($opts, $nl_flags, $defaults, $nl) = @_;
 

bld/namelist_files/namelist_defaults_ctsm.xml

@@ -57,6 +57,8 @@ attributes from the config_cache.xml file (with keys converted to upper-case).
 <spinup_state clm_accelerated_spinup="on"  use_fates=".true." phys="clm4_5" >1</spinup_state>
 <spinup_state clm_accelerated_spinup="off"                               >0</spinup_state>
 
+<for_testing_run_ncdiopio_tests>.false.</for_testing_run_ncdiopio_tests>
+
 <!-- In accelerated spinup mode reduce the amount of history output -->
 <hist_empty_htapes clm_accelerated_spinup="on">.true.</hist_empty_htapes>
 <hist_fincl1 clm_accelerated_spinup="on" use_cn=".true." use_cndv=".true."

bld/namelist_files/namelist_definition_ctsm.xml

@@ -941,6 +941,12 @@ Whether to use subgrid fluxes for snow
 Whether snow on the vegetation canopy affects the radiation/albedo calculations
 </entry>
 
+<entry id="for_testing_run_ncdiopio_tests" type="logical" category="default_settings"
+       group="clm_inparm" >
+Whether to run some tests of ncdio_pio as part of the model run. This is
+typically only used in automated tests.
+</entry>
+
 <!-- ========================================================================================  -->
 <!-- Former CPP tokens -->
 <!-- ========================================================================================  -->

cime_config/buildlib

@@ -115,6 +115,7 @@ def _main_func():
                      os.path.join(lnd_root,"src","soilbiogeochem"),
                      os.path.join(lnd_root,"src","dyn_subgrid"),
                      os.path.join(lnd_root,"src","init_interp"),
+                     os.path.join(lnd_root,"src","self_tests"),
                      os.path.join(lnd_root,"src","fates"),
                      os.path.join(lnd_root,"src","fates","main"),
                      os.path.join(lnd_root,"src","fates","biogeophys"),

cime_config/testdefs/testlist_clm.xml

@@ -2331,6 +2331,19 @@ for ERS test as otherwise it won't work for a sub-day test (no need to run this
     </machines>
   </test>
 
+  <test name="SMS_D_Ln1" grid="f10_f10_musgs" compset="I2000Clm50BgcCropQianRsGs" testmods="clm-run_self_tests">
+    <machines>
+
+      <machine name="izumi" compiler="intel" category="aux_clm">
+        <options>
+          <option name="wallclock">0:20:00</option>
+          <option name="comment">Include a test that triggers runtime self-tests. The grid and compset aren't very important here, but we do want more than a single-point test so that we can run on more than one processor; we use Qian atm forcing to facilitate running this test on small systems (to avoid large input data needs). The self-tests are run in initialization, so we only need to run for a single time step.</option>
+        </options>
+      </machine>
+
+    </machines>
+  </test>
+
   <test name="FUNITCTSM_P1x1" grid="f10_f10_musgs" compset="I2000Clm50SpGs">
     <machines>
       <machine name="cheyenne" compiler="intel" category="aux_clm">

cime_config/testdefs/testmods_dirs/clm/run_self_tests/README

@@ -0,0 +1,5 @@
+The purpose of this testmod directory is to trigger the runtime
+self-tests. This runs a suite of unit/integration tests.
+
+We use cold start so that we can get through initialization faster,
+since how we initialize the model is unimportant for these self-tests.

cime_config/testdefs/testmods_dirs/clm/run_self_tests/shell_commands

@@ -0,0 +1,3 @@
+#!/bin/bash
+./xmlchange CLM_FORCE_COLDSTART="on"
+

cime_config/testdefs/testmods_dirs/clm/run_self_tests/user_nl_clm

@@ -0,0 +1 @@
+for_testing_run_ncdiopio_tests = .true.

doc/ChangeLog

@@ -1,4 +1,135 @@
 ===============================================================
+Tag name:  ctsm5.1.dev010
+Originator(s):  sacks (Bill Sacks), jedwards (Jim Edwards)
+Date:  Fri Oct 23 14:36:03 MDT 2020
+One-line Summary: Fix reading netcdf variable into a different data type
+
+Purpose of changes
+------------------
+
+Main change is to fix the reading of a variable that is one type on a
+NetCDF file but a different data type in memory. Previously, this could
+lead to memory corruption (not just of the variable in question, but
+other variables as well). This was not causing any problems in
+out-of-the-box configurations, but it sometimes tripped up some
+developers when they added new fields on datasets.
+
+In addition, this adds new self-test code to test ncdio_pio. This module
+is hard to get under unit test, but we want to have some tests of it, to
+pick up problems like the one fixed in this tag. So as a compromise
+solution for now, I have introduced unit test-like tests that are built
+into a standard build of CTSM and run in a standard run if a given
+namelist flag is set. A better long-term solution would be to integrate
+these tests into the pFUnit-based unit testing framework, but that would
+take some work. I have added tests of the new code, and a bit of testing
+of other code in ncdio_pio, but there's still a lot in ncdio_pio that is
+not tested. I figure we can gradually add tests as we make changes to
+ncdio_pio.
+
+Finally, an unrelated change to the unit test build: files generated by
+genf90 now appear in the unit test build directory rather than the
+source tree.
+
+Bugs fixed or introduced
+------------------------
+
+Issues fixed (include CTSM Issue #):
+- Resolves ESCOMP/CTSM#1091 (Memory corruption when reading a netcdf
+  variable into a variable of a different type, at least with PIO1)
+- Resolves ESCOMP/CTSM#1188 (In unit test build: files generated by
+  genf90 should go in build dir rather than source dir)
+
+Significant changes to scientifically-supported configurations
+--------------------------------------------------------------
+
+Does this tag change answers significantly for any of the following physics configurations?
+(Details of any changes will be given in the "Answer changes" section below.)
+
+    [Put an [X] in the box for any configuration with significant answer changes.]
+
+[ ] clm5_1
+
+[ ] clm5_0
+
+[ ] ctsm5_0-nwp
+
+[ ] clm4_5
+
+Notes of particular relevance for users
+---------------------------------------
+
+Caveats for users (e.g., need to interpolate initial conditions): none
+
+Changes to CTSM's user interface (e.g., new/renamed XML or namelist variables):
+- new namelist variable, for_testing_run_ncdiopio_tests
+
+Changes made to namelist defaults (e.g., changed parameter values): none
+
+Changes to the datasets (e.g., parameter, surface or initial files): none
+
+Substantial timing or memory changes: none
+
+Notes of particular relevance for developers: (including Code reviews and testing)
+---------------------------------------------
+NOTE: Be sure to review the steps in README.CHECKLIST.master_tags as well as the coding style in the Developers Guide
+
+Caveats for developers (e.g., code that is duplicated that requires double maintenance):
+- Ideally the ncdio_pio self-tests would be recoded as true unit tests,
+  leveraging the pFUnit framework. This would get them out of the main
+  source code, and would give other benefits that come with a real unit
+  testing framework, such as that a single failure doesn't abort the
+  entire test suite.
+- The existence of tests for ncdio_pio should NOT be taken as a sign
+  that all (or even most) of this module is now under unit test: in
+  fact, much of it is not covered yet by my new testing module.
+
+Changes to tests or testing:
+- Added a test that triggers for_testing_run_ncdiopio_tests
+
+CTSM testing:
+
+ [PASS means all tests PASS and OK means tests PASS other than expected fails.]
+
+  build-namelist tests:
+
+    cheyenne - not run
+
+  tools-tests (test/tools):
+
+    cheyenne - not run
+
+  PTCLM testing (tools/shared/PTCLM/test):
+
+    cheyenne - not run
+
+  python testing (see instructions in python/README.md; document testing done):
+
+    (any machine) - not run
+
+  regular tests (aux_clm):
+
+    cheyenne ---- pass
+    izumi ------- pass
+
+If the tag used for baseline comparisons was NOT the previous tag, note that here:
+
+
+Answer changes
+--------------
+
+Changes answers relative to baseline: NO
+
+Detailed list of changes
+------------------------
+
+List any externals directories updated (cime, rtm, mosart, cism, fates, etc.): none
+
+Pull Requests that document the changes (include PR ids):
+https://github.com/ESCOMP/CTSM/pull/1189
+https://github.com/ESCOMP/CTSM/pull/1176 (old version, superseded by 1189)
+
+===============================================================
+===============================================================
 Tag name:  ctsm5.1.dev009
 Originator(s):  negins (Negin Sobhani,UCAR/TSS,303-497-1224)
 Date: Wed Oct 21 11:14:25 MDT 2020

doc/ChangeSum

@@ -1,5 +1,6 @@
 Tag                   Who      Date  Summary
 ============================================================================================================================
+  ctsm5.1.dev010 sacks/je 10/23/2020 Fix reading netcdf variable into a different data type
   ctsm5.1.dev009   negins 10/21/2020 BFB changes for Perturbed Parameter Ensemble and  Hydraulic redistribution
   ctsm5.1.dev008     erik 10/07/2020 Two answer changes: Clm45/50/51 with crop, and for 2000Clm51
   ctsm5.1.dev007    sacks 10/06/2020 CNFire: btran2 fixes and general cleanup

src/CMakeLists.txt

@@ -35,6 +35,7 @@ add_subdirectory(${CLM_ROOT}/src/biogeophys clm_biogeophys)
 add_subdirectory(${CLM_ROOT}/src/dyn_subgrid clm_dyn_subgrid)
 add_subdirectory(${CLM_ROOT}/src/main clm_main)
 add_subdirectory(${CLM_ROOT}/src/init_interp clm_init_interp)
+add_subdirectory(${CLM_ROOT}/src/self_tests clm_self_tests)
 
 # Add general unit test directories (stubbed out files, etc.)
 add_subdirectory(unit_test_stubs)
@@ -105,3 +106,4 @@ add_subdirectory(${CLM_ROOT}/src/soilbiogeochem/test clm_soilbiogeochem_test)
 add_subdirectory(${CLM_ROOT}/src/dyn_subgrid/test clm_dyn_subgrid_test)
 add_subdirectory(${CLM_ROOT}/src/main/test clm_main_test)
 add_subdirectory(${CLM_ROOT}/src/init_interp/test clm_init_interp_test)
+add_subdirectory(${CLM_ROOT}/src/self_tests/test clm_self_tests_test)

src/dyn_subgrid/CMakeLists.txt

@@ -7,7 +7,7 @@ set(genf90_files
   dynVarTimeUninterpMod.F90.in
   )
 
-process_genf90_source_list("${genf90_files}" ${CMAKE_CURRENT_SOURCE_DIR} clm_genf90_sources)
+process_genf90_source_list("${genf90_files}" ${CMAKE_CURRENT_BINARY_DIR} clm_genf90_sources)
 
 sourcelist_to_parent(clm_genf90_sources)
 

src/main/clm_initializeMod.F90

@@ -26,6 +26,7 @@ module clm_initializeMod
   use filterMod       , only : allocFilters, filter, filter_inactive_and_active
   use FatesInterfaceMod, only : set_fates_global_elements
   use dynSubgridControlMod, only: dynSubgridControl_init, get_reset_dynbal_baselines
+  use SelfTestDriver, only : self_test_driver
 
   use clm_instMod
   use SoilMoistureStreamMod, only : PrescribedSoilMoistureInit
@@ -261,6 +262,9 @@ subroutine initialize1(dtime, gindex_ocn)
        call ch4conrd()
     end if
 
+    ! Run any requested self-tests
+    call self_test_driver(bounds_proc)
+
     ! Deallocate surface grid dynamic memory for variables that aren't needed elsewhere.
     ! Some things are kept until the end of initialize2; urban_valid is kept through the
     ! end of the run for error checking.

src/main/clm_varctl.F90

@@ -53,6 +53,9 @@ module clm_varctl
   !true => no valid land points -- do NOT run
   logical, public :: noland = .false.                                    
 
+  ! true => run tests of ncdio_pio
+  logical, public :: for_testing_run_ncdiopio_tests = .false.
+
   ! Hostname of machine running on
   character(len=256), public :: hostname = ' '                           
 

src/main/controlMod.F90

@@ -139,7 +139,7 @@ subroutine control_init(dtime)
 
     ! CLM namelist settings
 
-    namelist /clm_inparm / &
+    namelist /clm_inparm/ &
          fatmlndfrc, finidat, nrevsn, &
          finidat_interp_dest, &
          use_init_interp, compname
@@ -202,7 +202,7 @@ subroutine control_init(dtime)
          albice, soil_layerstruct_predefined, soil_layerstruct_userdefined, &
          soil_layerstruct_userdefined_nlevsoi, use_subgrid_fluxes, snow_cover_fraction_method, &
          irrigate, run_zero_weight_urban, all_active, &
-         crop_fsat_equals_zero
+         crop_fsat_equals_zero, for_testing_run_ncdiopio_tests
 
     ! vertical soil mixing variables
     namelist /clm_inparm/  &
@@ -656,6 +656,9 @@ subroutine control_spmd()
     ! Crop saturated excess runoff
     call mpi_bcast(crop_fsat_equals_zero, 1, MPI_LOGICAL, 0, mpicom, ier)
 
+    ! Whether to run tests of ncdio_pio
+    call mpi_bcast(for_testing_run_ncdiopio_tests, 1, MPI_LOGICAL, 0, mpicom, ier)
+
     ! Landunit generation
     call mpi_bcast(create_crop_landunit, 1, MPI_LOGICAL, 0, mpicom, ier)