Make "single-point" tests actually include multiple points

Include three points, where point 2 is the point of interest and points
1 and 3 just exist to give some (but not complete) assurance that we
don't accidentally do whole-array assignments (foo = bar(p)) where we
meant to do single-element assignments (foo(p) = bar(p)) (because, if we
did such an assignment, this would overwrite the values in points 1 and
3, which otherwise should remain unchanged).

src/biogeophys/test/Irrigation_test/README

@@ -20,9 +20,15 @@ set up these tests:
 Because of these considerations, it was not straightforward to pull out routines
 that could operate on a single point, and just do the testing on these
 single-point routines. So instead, I am just testing the public, multi-point
-routines. However, for simplicity, most of my tests just use a single point in
+routines. But for simplicity, most of my tests just focus on a single point in
 the arrays - and then I have just enough multi-point tests to ensure that the
-routines truly do work with multiple points.
+routines truly do work with multiple points. However, even the "single-point"
+tests actually include three points, where point 2 is the point of interest and
+points 1 and 3 just exist to give some (but not complete) assurance that we
+don't accidentally do whole-array assignments (foo = bar(p)) where we meant to
+do single-element assignments (foo(p) = bar(p)) (because, if we did such an
+assignment, this would overwrite the values in points 1 and 3, which otherwise
+should remain unchanged).
 
 Furthermore, I have been influenced lately by advice to "test behavior, not
 methods", and to test through the public interface. And in this case, it

src/biogeophys/test/Irrigation_test/test_irrigation.pf

@@ -18,7 +18,7 @@ module test_irrigation
   use ColumnType    , only : col
   use GridcellType  , only : grc
   use pftconMod , only : pftcon
-  use unittestSimpleSubgridSetupsMod, only : setup_single_veg_patch
+  use unittestSimpleSubgridSetupsMod, only : setup_ncells_single_veg_patch
   use unittestFilterBuilderMod, only : filter_from_range
 
   implicit none
@@ -28,9 +28,11 @@ module test_irrigation
   integer , parameter :: dtime = 1800  ! model time step, seconds
   integer , parameter :: irrig_start = 21600
   integer , parameter :: pft_type = 1  ! pft type in target patch for single-patch tests
+  real(r8), parameter :: flux_uninit = 12345._r8
 
   @TestCase
   type, extends(TestCase) :: TestIrrigation
+     logical :: is_single_point = .false.  ! true if this is a single-point test (and so single_p, single_c and single_g are defined)
      integer :: single_p  ! for single-point tests: patch of interest (for non-single-point tests, this is undefined)
      integer :: single_c  ! for single-point tests: column of interest (for non-single-point tests, this is undefined)
      integer :: single_g  ! for single-point tests: gridcell of interest (for non-single-point tests, this is undefined)
@@ -96,6 +98,10 @@ module test_irrigation
      ! Assert that total irrigation withdrawal and application are both greater than zero
      ! for a given patch
      procedure :: assertTotalIrrigationGreaterThanZero
+
+     ! In a single-point test, assert that the fluxes in the non-points-of-interest
+     ! remain unchanged from their initial values
+     procedure :: assertOtherPointsUnchanged
   end type TestIrrigation
 
   real(r8), parameter :: mm_times_km2_to_m3 = 1.e3_r8
@@ -147,14 +153,18 @@ contains
   end subroutine tearDown
 
   subroutine setupSinglePatch(this)
-    ! Sets up grid with single veg patch; also sets up this%filter appropriately
+    ! Sets up a grid for testing a single veg patch. This actually sets up three grid
+    ! cells, each with a single veg patch; the 2nd grid cell / patch is the point of
+    ! interest. Also sets up this%filter to just include the 2nd patch. This way, we can
+    ! ensure that values in points 1 and 3 remain unchanged.
     class(TestIrrigation), intent(inout) :: this
 
-    call setup_single_veg_patch(pft_type=pft_type)
-    this%single_p = bounds%begp
-    this%single_c = bounds%begc
-    this%single_g = bounds%begg
-    call filter_from_range(start=bounds%begp, end=bounds%endp, numf=this%numf, filter=this%filter)
+    call setup_ncells_single_veg_patch(ncells=3, pft_type=pft_type)
+    this%is_single_point = .true.
+    this%single_p = bounds%begp + 1
+    this%single_c = bounds%begc + 1
+    this%single_g = bounds%begg + 1
+    call filter_from_range(start=this%single_p, end=this%single_p, numf=this%numf, filter=this%filter)
   end subroutine setupSinglePatch
 
   !-----------------------------------------------------------------------
@@ -206,6 +216,8 @@ contains
     !
     ! !LOCAL VARIABLES:
     integer :: c,j
+    real(r8) :: flux_init_p(bounds%begp:bounds%endp)
+    real(r8) :: flux_init_c(bounds%begc:bounds%endc)
     logical :: limit_irrigation_if_rof_enabled
     integer :: l_maxpft
     integer :: l_nlevirrig
@@ -258,16 +270,25 @@ contains
 
     ! Allocate fluxes output from irrigation routines. Note that, in the production code,
     ! these are initialized to 0 in InitCold.
-    allocate(this%waterflux%qflx_sfc_irrig_patch(bounds%begp:bounds%endp), source=0._r8)
-    allocate(this%waterflux%qflx_sfc_irrig_col(bounds%begc:bounds%endc), source=0._r8)
-    allocate(this%waterflux%qflx_gw_uncon_irrig_patch(bounds%begp:bounds%endp), source=0._r8)
-    allocate(this%waterflux%qflx_gw_uncon_irrig_col(bounds%begc:bounds%endc), source=0._r8)
-    allocate(this%waterflux%qflx_gw_con_irrig_patch(bounds%begp:bounds%endp), source=0._r8)
-    allocate(this%waterflux%qflx_gw_con_irrig_col(bounds%begc:bounds%endc), source=0._r8)
-    allocate(this%waterflux%qflx_irrig_drip_patch(bounds%begp:bounds%endp), source=0._r8)
-    allocate(this%waterflux%qflx_irrig_drip_col(bounds%begc:bounds%endc), source=0._r8)
-    allocate(this%waterflux%qflx_irrig_sprinkler_patch(bounds%begp:bounds%endp), source=0._r8)
-    allocate(this%waterflux%qflx_irrig_sprinkler_col(bounds%begc:bounds%endc), source=0._r8)
+    if (this%is_single_point) then
+       flux_init_p(:) = flux_uninit
+       flux_init_p(this%single_p) = 0._r8
+       flux_init_c(:) = flux_uninit
+       flux_init_c(this%single_c) = 0._r8
+    else
+       flux_init_p(:) = 0._r8
+       flux_init_c(:) = 0._r8
+    end if
+    allocate(this%waterflux%qflx_sfc_irrig_patch(bounds%begp:bounds%endp), source=flux_init_p)
+    allocate(this%waterflux%qflx_sfc_irrig_col(bounds%begc:bounds%endc), source=flux_init_c)
+    allocate(this%waterflux%qflx_gw_uncon_irrig_patch(bounds%begp:bounds%endp), source=flux_init_p)
+    allocate(this%waterflux%qflx_gw_uncon_irrig_col(bounds%begc:bounds%endc), source=flux_init_c)
+    allocate(this%waterflux%qflx_gw_con_irrig_patch(bounds%begp:bounds%endp), source=flux_init_p)
+    allocate(this%waterflux%qflx_gw_con_irrig_col(bounds%begc:bounds%endc), source=flux_init_c)
+    allocate(this%waterflux%qflx_irrig_drip_patch(bounds%begp:bounds%endp), source=flux_init_p)
+    allocate(this%waterflux%qflx_irrig_drip_col(bounds%begc:bounds%endc), source=flux_init_c)
+    allocate(this%waterflux%qflx_irrig_sprinkler_patch(bounds%begp:bounds%endp), source=flux_init_p)
+    allocate(this%waterflux%qflx_irrig_sprinkler_col(bounds%begc:bounds%endc), source=flux_init_c)
 
     ! Set inputs to irrigation routines
     allocate(this%elai(bounds%begp:bounds%endp), source=10._r8)
@@ -500,19 +521,32 @@ contains
          rof_prognostic = .true.)
 
     ! The expectation is that the filter used in CalcIrrigationNeeded is a subset of the
-    ! filter used in ApplyIrrigation. In order to (a) keep the unit tests simpler, and (b)
-    ! ensure that it works for the ApplyIrrigation filter to have points not in the
+    ! filter used in ApplyIrrigation. (The one situation where the ApplyIrrigation filter
+    ! might include points not in the CalcIrrigationNeeded filter is if a point that was
+    ! inactive at the time of the CalcIrrigationNeeded call has become active for the
+    ! ApplyIrrigation call; in this case, if there were still some irrigation time steps
+    ! left when it had become inactive, then ApplyIrrigation might start re-irrigating
+    ! there. But this behavior is probably okay; in any case, it doesn't seem worth
+    ! testing for.)
+    !
+    ! In order to (a) keep the unit tests simpler, and (b) ensure that it
+    ! works for the ApplyIrrigation filter to have points not in the
     ! CalcIrrigationNeededFilter, we send ApplyIrrigation a filter that includes all
-    ! points. (The one situation where the ApplyIrrigation filter might include points not
-    ! in the CalcIrrigationNeeded filter is if a point that was inactive at the time of
-    ! the CalcIrrigationNeeded call has become active for the ApplyIrrigation call; in
-    ! this case, if there were still some irrigation time steps left when it had become
-    ! inactive, then ApplyIrrigation might start re-irrigating there. But this behavior
-    ! is probably okay; in any case, it doesn't seem worth testing for.)
-    call filter_from_range(start=bounds%begp, end=bounds%endp, &
-         numf=apply_nump, filter=apply_filterp)
-    call filter_from_range(start=bounds%begc, end=bounds%endc, &
-         numf=apply_numc, filter=apply_filterc)
+    ! points. However, for "single-point" tests, we just use the single actual point, to
+    ! avoid resetting fluxes outside the point-of-interest to 0 (because the tests of
+    ! maintaining the original value in those points are slightly stronger if we use
+    ! non-zero values).
+    if (this%is_single_point) then
+       call filter_from_range(start=this%single_p, end=this%single_p, &
+            numf=apply_nump, filter=apply_filterp)
+       call filter_from_range(start=this%single_c, end=this%single_c, &
+            numf=apply_numc, filter=apply_filterc)
+    else
+       call filter_from_range(start=bounds%begp, end=bounds%endp, &
+            numf=apply_nump, filter=apply_filterp)
+       call filter_from_range(start=bounds%begc, end=bounds%endc, &
+            numf=apply_numc, filter=apply_filterc)
+    end if
 
     call this%irrigation%ApplyIrrigation( &
          bounds = bounds, &
@@ -701,13 +735,59 @@ contains
 
   end subroutine assertTotalIrrigationGreaterThanZero
 
+  !-----------------------------------------------------------------------
+  subroutine assertOtherPointsUnchanged(this)
+    !
+    ! !DESCRIPTION:
+    ! In a single-point test, assert that the fluxes in the non-points-of-interest remain
+    ! unchanged from their initial values.
+    !
+    ! This provides some (though not complete) measure of assurance that we do not have
+    ! code that sets whole arrays when we mean to just assign to a given element - i.e.,
+    ! that we do not have:
+    !    foo = bar(p)
+    ! when we mean:
+    !    foo(p) = bar(p)
+    !
+    ! !ARGUMENTS:
+    class(TestIrrigation), intent(in) :: this
+    !
+    ! !LOCAL VARIABLES:
+    integer :: p
+
+    character(len=*), parameter :: subname = 'assertOtherPointsUnchanged'
+    !-----------------------------------------------------------------------
+
+    ! This should only be called in a "single-point" test
+    @assertTrue(this%is_single_point)
+
+    ! It's enough to check the patch values, since the column values are set from the
+    ! patch values
+    do p = bounds%begp, bounds%endp
+       if (p == this%single_p) then
+          cycle
+       end if
+       @assertEqual(flux_uninit, this%waterflux%qflx_sfc_irrig_patch(p))
+       @assertEqual(flux_uninit, this%waterflux%qflx_gw_uncon_irrig_patch(p))
+       @assertEqual(flux_uninit, this%waterflux%qflx_gw_con_irrig_patch(p))
+
+       @assertEqual(flux_uninit, this%waterflux%qflx_irrig_drip_patch(p))
+       @assertEqual(flux_uninit, this%waterflux%qflx_irrig_sprinkler_patch(p))
+    end do
+
+  end subroutine assertOtherPointsUnchanged
 
   ! ========================================================================
   ! Begin actual tests
   ! ========================================================================
 
   ! ------------------------------------------------------------------------
-  ! Tests on a single patch
+  ! Tests on a single patch.
+  !
+  ! Note that these actually use three grid cells, where grid cell #2 is the grid cell of
+  ! interest, and the other two just serve to help ensure that we don't have code that
+  ! assigns to whole arrays (foo = bar(p)) when we really mean to just assign to a single
+  ! element (foo(p) = bar(p)).
   ! ------------------------------------------------------------------------
 
   @Test
@@ -727,6 +807,7 @@ contains
     call this%computeDeficits(deficits)
     expected = sum(deficits(this%single_p,1:nlevsoi)) / this%irrigation_params%irrig_length
     call this%assertTotalIrrigationEquals(this%single_p, expected)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine irrigation_flux_is_correct
 
@@ -752,6 +833,7 @@ contains
     ! Make sure all irrigation comes from drip (both patch and column-level)
     @assertEqual(expected, this%waterflux%qflx_irrig_drip_patch(this%single_p), tolerance=tol)
     @assertEqual(expected, this%waterflux%qflx_irrig_drip_col(this%single_c), tolerance=tol)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine drip
 
@@ -777,6 +859,7 @@ contains
     ! Make sure all irrigation comes from sprinkler (both patch and column-level)
     @assertEqual(expected, this%waterflux%qflx_irrig_sprinkler_patch(this%single_p), tolerance=tol)
     @assertEqual(expected, this%waterflux%qflx_irrig_sprinkler_col(this%single_c), tolerance=tol)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine sprinkler
 
@@ -794,6 +877,7 @@ contains
 
     ! Check result
     call this%assertTotalIrrigationZero(this%single_p)
+    call this%assertOtherPointsUnchanged()
   end subroutine no_irrigation_for_wet_soil
 
   @Test
@@ -824,6 +908,7 @@ contains
     @assertLessThan(0._r8, expected)
     ! Here is the main assertion:
     call this%assertTotalIrrigationEquals(this%single_p, expected)
+    call this%assertOtherPointsUnchanged()
   end subroutine surplus_offsets_deficit
 
   @Test
@@ -842,6 +927,7 @@ contains
 
     ! Check result
     call this%assertTotalIrrigationZero(this%single_p)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine no_irrigation_for_unirrigated_pfts
 
@@ -859,6 +945,7 @@ contains
 
     ! Check result
     call this%assertTotalIrrigationZero(this%single_p)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine no_irrigation_for_lai0
 
@@ -886,6 +973,7 @@ contains
 
     ! Check result
     call this%assertTotalIrrigationZero(this%single_p)
+    call this%assertOtherPointsUnchanged()
   end subroutine no_irrigation_for_soil_moisture_above_threshold
 
   @Test
@@ -903,6 +991,7 @@ contains
 
     ! Check result
     call this%assertTotalIrrigationZero(this%single_p)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine no_irrigation_at_wrong_time
 
@@ -940,6 +1029,7 @@ contains
     ! Make sure that all irrigation comes as surface irrigation (patch and column-level)
     @assertEqual(expected, this%waterflux%qflx_sfc_irrig_patch(this%single_p), tolerance=tol)
     @assertEqual(expected, this%waterflux%qflx_sfc_irrig_col(this%single_c), tolerance=tol)
+    call this%assertOtherPointsUnchanged()
   end subroutine unlimited_irrigation_for_non_limiting_volr
 
   @Test
@@ -976,6 +1066,7 @@ contains
     ! Make sure that all irrigation comes as surface irrigation (patch and column-level)
     @assertEqual(expected, this%waterflux%qflx_sfc_irrig_patch(this%single_p), tolerance=tol)
     @assertEqual(expected, this%waterflux%qflx_sfc_irrig_col(this%single_c), tolerance=tol)
+    call this%assertOtherPointsUnchanged()
   end subroutine limited_irrigation_for_limiting_volr_no_groundwater
 
   @Test
@@ -1020,6 +1111,7 @@ contains
     @assertEqual(volr_limited_irrig_rate, this%waterflux%qflx_sfc_irrig_col(this%single_c), tolerance=tol)
     @assertEqual(expected_gw_uncon, this%waterflux%qflx_gw_uncon_irrig_patch(this%single_p), tolerance=tol)
     @assertEqual(expected_gw_uncon, this%waterflux%qflx_gw_uncon_irrig_col(this%single_c), tolerance=tol)
+    call this%assertOtherPointsUnchanged()
   end subroutine limiting_volr_with_groundwater_uncon
 
   @Test
@@ -1078,6 +1170,7 @@ contains
     @assertEqual(expected_gw_uncon, this%waterflux%qflx_gw_uncon_irrig_col(this%single_c), tolerance=tol)
     @assertEqual(expected_gw_con, this%waterflux%qflx_gw_con_irrig_patch(this%single_p), tolerance=tol)
     @assertEqual(expected_gw_con, this%waterflux%qflx_gw_con_irrig_col(this%single_c), tolerance=tol)
+    call this%assertOtherPointsUnchanged()
   end subroutine limiting_volr_with_groundwater_uncon_and_con
 
   @Test
@@ -1102,6 +1195,7 @@ contains
 
     ! Check result
     call this%assertTotalIrrigationEquals(this%single_p, expected)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine irrigation_continues_at_same_rate_for_multiple_time_steps
 
@@ -1129,6 +1223,7 @@ contains
     ! Ensure that irrigation flux goes to 0 in the following time step
     call this%calculateAndApplyIrrigation()
     call this%assertTotalIrrigationZero(this%single_p)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine irrigation_continues_for_correct_number_of_time_steps
 
@@ -1167,6 +1262,7 @@ contains
     @assertLessThan(0._r8, expected)
     ! Here's the main assertion:
     call this%assertTotalIrrigationEquals(this%single_p, expected)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine irrigation_flux_is_correct_on_second_day
 
@@ -1190,6 +1286,7 @@ contains
     ! Now on to the real assertion
     expected = sum(deficits(this%single_p,1:nlevirrig)) / this%irrigation_params%irrig_length
     call this%assertTotalIrrigationEquals(this%single_p, expected)
+    call this%assertOtherPointsUnchanged()
   end subroutine irrigation_excludes_deep_layers
 
   @Test
@@ -1211,6 +1308,7 @@ contains
     call this%computeDeficits(deficits)
     expected = sum(deficits(this%single_p,1:(nlevsoi-1))) / this%irrigation_params%irrig_length
     call this%assertTotalIrrigationEquals(this%single_p, expected)
+    call this%assertOtherPointsUnchanged()
   end subroutine irrigation_excludes_bedrock_layers
 
   @Test
@@ -1227,6 +1325,7 @@ contains
 
     ! Check result
     call this%assertTotalIrrigationZero(this%single_p)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine no_irrigation_for_frozen_soil
 
@@ -1249,6 +1348,7 @@ contains
     ! Only include deficit from top layer, since 2nd layer is frozen
     expected = deficits(this%single_p, 1) / this%irrigation_params%irrig_length
     call this%assertTotalIrrigationEquals(this%single_p, expected)
+    call this%assertOtherPointsUnchanged()
 
   end subroutine no_irrigation_below_frozen_soil_layer