Merge branch 'btran2incnfire_movecall'

CNFire: btran2 fixes and general cleanup (1) Call routine to calculate fire's btran2 from CNFireArea; this has a few advantages: - It makes the logic of CNFireArea more clear (rather than depending on a btran2 variable that is calculated from some other part of the code) - This avoids having the biogeophysics depend on the biogeochemistry - This lets us avoid doing this btran calc if using no-fire – or other, future, fire methods that don't need it Note regarding testing: In the initial step, I kept this calculation dependent on a saved version of h2osoi_vol to avoid changing answers; I changed this in the answer changes in step (2), as noted below. (2) Answer-changing fixes to CNFire's btran2 calculation and use: (a) Calculate fire btran2 using updated h2osoi_vol (this is an answer-changing cleanup step from (1)) (b) TEMPORARY CHANGE (reverted in the cleanup in (3)): Reinitialize fire btran2 to spval for all patches in each time step, so that the fire code isn't trying to use btran2 from earlier time steps for patches that were in the exposed veg filter at one point but no longer are. One implication of this is that, if there is currently no exposed veg on a column, the new code leads to the block of code that forces fire_m = 0 (because wtlf will be 0). Previously, in contrast, it looks like fire_m was allowed to be non-zero even if there is currently no exposed veg, because btran2 and wtlf were accumulated if a patch ever was exposed in the past. (c) Limit fire btran2 to be <= 1, rather than letting it be slightly greater than 1. (Due to a conditional in CNFireArea, these slightly-greater-tan-1 values were being ignored when computing btran_col, rather than averaging in a 1 value.) (3) Non-answer-changing fire code cleanup: (a) Cleanup of the btran2 fixes, including reverting the TEMPORARY CHANGE noted in (2b), instead relying on a better mechanism: just doing the calculations of btran_col and wtlf over the exposedvegp filter. Also, get rid of the checks for shr_infnan_isnan(btran2(p)) and btran2(p) <= 1 (allowed by the other changes in (2) and (3)). (b) Set btran2 to 0 over non-exposed-veg points: this changes answers for the BTRAN2 diagnostic field, but nothing else. (This follows what is done for the standard BTRAN.) (c) Move calc_fire_root_wetness for CNFireLi2021 into the base type to avoid future bugs (assuming that the next fire module will extend the base class but will also want to use this new version of calc_fire_root_wetness). (d) Change fire looping structure to be more standard (4) Remove some very expensive tests from aux_clm, putting some in the new ctsm_sci test list instead (5) A bit of other minor cleanup Issues fixed: - Resolves #1139 (Decrease expense of ne0ARCTICGRISne30x8 test and C96 tests) - Resolves #1153 (Fire btran2 is only computed for exposed veg patches, but is used over all veg patches) - Resolves #1170 (CNFire code: btran2 should not be skipped when it's greater than 1) - Partially addresses #1142 (Add ctsm_sci test list that would be used for releases and making sure important resolutions run well)
ESCOMP · Oct 6, 2020 · a17cc14 · a17cc14
2 parents d6eccd1 + d2f5705
commit a17cc14
Show file tree

Hide file tree

Showing 18 changed files with 1,134 additions and 971 deletions.
diff --git a/cime_config/testdefs/testlist_clm.xml b/cime_config/testdefs/testlist_clm.xml
@@ -55,13 +55,13 @@
       <option name="wallclock">00:20:00</option>
     </options>
   </test>
-  <test name="ERI_D_Ld9" grid="C96_t061" compset="I2000Clm50SpRsGs" testmods="clm/default">
+  <test name="SMS_Ln9" grid="C96_t061" compset="I2000Clm50SpRsGs" testmods="clm/default">
     <machines>
-      <machine name="cheyenne" compiler="intel" category="aux_clm"/>
+      <machine name="cheyenne" compiler="intel" category="ctsm_sci"/>
     </machines>
     <options>
       <option name="wallclock">00:60:00</option>
-      <option name="comment">Ran a basic test for fv3 C96 standard resolution</option>
+      <option name="comment">We have one C96 test in aux_clm; this is another that uses a different compset. No need to run this additional C96 test with every tag, but include it in the less frequent ctsm_sci testing.</option>
     </options>
   </test>
   <test name="ERI_D_Ld9" grid="ne30_g17" compset="I2000Clm50BgcCruGs" testmods="clm/vrtlay">
@@ -301,13 +301,13 @@
       <option name="wallclock">00:60:00</option>
     </options>
   </test>
-  <test name="ERP_D_Ld5" grid="C96_C96_mg17" compset="IHistClm50BgcCrop" testmods="clm/allActive">
+  <test name="SMS_Ln9" grid="C96_C96_mg17" compset="IHistClm50BgcCrop" testmods="clm/default">
     <machines>
       <machine name="cheyenne" compiler="intel" category="aux_clm"/>
     </machines>
     <options>
       <option name="wallclock">00:20:00</option>
-      <option name="comment">Use a transient compset so we allocate and run all PFTs (non-transient cases only allocate memory for non-zero-weight PFTs)</option>
+      <option name="comment">Want one C96 test in the aux_clm test suite; just a short smoke test to make sure it can get off the ground</option>
     </options>
   </test>
   <test name="SMS_Ld5" grid="f09_g17" compset="IHistClm50BgcCrop" testmods="clm/default">
@@ -1019,13 +1019,13 @@
       <option name="comment"  >Test transient PFTs (via HIST) in conjunction with changing glacier area. This test also covers the reset_dynbal_baselines option. CISM is not answer preserving across processor changes, but short test length should be OK.</option>
     </options>
   </test>
-  <test name="ERS_D_Ld10" grid="C96_C96_mg17" compset="IHistClm50Sp" testmods="clm/decStart">
+  <test name="SMS_Ln9" grid="C96_C96_mg17" compset="IHistClm50Sp" testmods="clm/decStart">
     <machines>
-      <machine name="cheyenne" compiler="intel" category="aux_clm"/>
+      <machine name="cheyenne" compiler="intel" category="ctsm_sci"/>
     </machines>
     <options>
       <option name="wallclock">00:20:00</option>
-      <option name="comment"  >test transient PFTs (via HIST) with a December start, reading 78-pft data and running with 16 pfts</option>
+      <option name="comment">We have one C96 test in aux_clm; this is another that uses a different compset. No need to run this additional C96 test with every tag, but include it in the less frequent ctsm_sci testing. decStart doesn't accomplish a lot in this very short test, but we're using it anyway to be consistent with other Hist tests.</option>
     </options>
   </test>
   <test name="ERS_D_Ld10" grid="f10_f10_musgs" compset="IHistClm50Sp" testmods="clm/collapse_pfts_78_to_16_decStart_f10">
@@ -1371,39 +1371,39 @@
   </test>
   <test name="ERS_Ln9" grid="ne0ARCTICne30x4_ne0ARCTICne30x4_mt12" compset="IHistClm50SpGs" testmods="clm/clm50cam6LndTuningMode_1979Start">
     <machines>
-      <machine name="cheyenne" compiler="intel" category="aux_clm"/>
+      <machine name="cheyenne" compiler="intel" category="ctsm_sci"/>
     </machines>
     <options>
       <option name="wallclock">00:20:00</option>
       <option name="comment"  >Run ARCTIC for transient case starting in 1979 as for AMIP CAM cases, be sure to run with stub GLC
-for ERS test as otherwise it won't work for a sub-day test"</option>
+for ERS test as otherwise it won't work for a sub-day test (no need to run this high core count test with every tag, but include it in the less frequent ctsm_sci testing)"</option>
     </options>
   </test>
   <test name="SMS_Ln9" grid="ne0ARCTICGRISne30x8_ne0ARCTICGRISne30x8_mt12" compset="IHistClm50Sp" testmods="clm/clm50cam6LndTuningMode_1979Start">
     <machines>
-      <machine name="cheyenne" compiler="intel" category="aux_clm"/>
+      <machine name="cheyenne" compiler="intel" category="ctsm_sci"/>
     </machines>
     <options>
-      <option name="wallclock">01:20:00</option>
-      <option name="comment"  >Run ARCTICGRIS for transient case starting in 1979 as for AMIP CAM cases"</option>
+      <option name="wallclock">00:20:00</option>
+      <option name="comment"  >Run ARCTICGRIS for transient case starting in 1979 as for AMIP CAM cases (no need to run this high core count test with every tag, but include it in the less frequent ctsm_sci testing)"</option>
     </options>
   </test>
   <test name="SMS_Ln9" grid="ne0ARCTICGRISne30x8_ne0ARCTICGRISne30x8_mt12" compset="ISSP585Clm50BgcCrop" testmods="clm/clm50cam6LndTuningMode">
     <machines>
-      <machine name="cheyenne" compiler="intel" category="aux_clm"/>
+      <machine name="cheyenne" compiler="intel" category="ctsm_sci"/>
     </machines>
     <options>
       <option name="wallclock">00:40:00</option>
-      <option name="comment"  >Run ARCTICGRIS for future transient case"</option>
+      <option name="comment"  >Run ARCTICGRIS for future transient case (do not run this expensive test with every tag, but include it in the less frequent ctsm_sci testing)"</option>
     </options>
   </test>
   <test name="SMS_Ln9" grid="ne0CONUSne30x8_ne0CONUSne30x8_mt12" compset="IHistClm50Sp" testmods="clm/clm50cam6LndTuningMode_2013Start">
     <machines>
-      <machine name="cheyenne" compiler="intel" category="aux_clm"/>
+      <machine name="cheyenne" compiler="intel" category="ctsm_sci"/>
     </machines>
     <options>
       <option name="wallclock">00:20:00</option>
-      <option name="comment"  >Run CONUS for transient case starting in 2013 as for CAM case"</option>
+      <option name="comment"  >Run CONUS for transient case starting in 2013 as for CAM case (no need to run this high core count test with every tag, but include it in the less frequent ctsm_sci testing)"</option>
     </options>
   </test>
   <test name="SMS_Ld5" grid="f09_g17" compset="IHistClm50Sp" testmods="clm/default">

diff --git a/doc/ChangeLog b/doc/ChangeLog
@@ -1,4 +1,214 @@
 ===============================================================
+Tag name:  ctsm5.1.dev007
+Originator(s):  sacks (Bill Sacks)
+Date:  Tue Oct  6 09:29:27 MDT 2020
+One-line Summary: CNFire: btran2 fixes and general cleanup
+
+Purpose of changes
+------------------
+
+(1) Call routine to calculate fire's btran2 from CNFireArea; this has a
+    few advantages:
+
+    - It makes the logic of CNFireArea more clear (rather than depending on
+      a btran2 variable that is calculated from some other part of the code)
+
+    - This avoids having the biogeophysics depend on the biogeochemistry
+
+    - This lets us avoid doing this btran calc if using no-fire – or other,
+      future, fire methods that don't need it
+
+    Note regarding testing: In the initial step, I kept this calculation
+    dependent on a saved version of h2osoi_vol to avoid changing
+    answers; I changed this in the answer changes in step (2), as noted
+    below.
+
+(2) Answer-changing fixes to CNFire's btran2 calculation and use:
+
+    (a) Calculate fire btran2 using updated h2osoi_vol (this is an
+        answer-changing cleanup step from (1))
+
+    (b) TEMPORARY CHANGE (reverted in the cleanup in (3)): Reinitialize
+        fire btran2 to spval for all patches in each time step, so that
+        the fire code isn't trying to use btran2 from earlier time steps
+        for patches that were in the exposed veg filter at one point but
+        no longer are.
+
+        One implication of this is that, if there is currently no
+        exposed veg on a column, the new code leads to the block of code
+        that forces fire_m = 0 (because wtlf will be 0). Previously, in
+        contrast, it looks like fire_m was allowed to be non-zero even
+        if there is currently no exposed veg, because btran2 and wtlf
+        were accumulated if a patch ever was exposed in the past.
+
+    (c) Limit fire btran2 to be <= 1, rather than letting it be slightly
+        greater than 1. (Due to a conditional in CNFireArea, these
+        slightly-greater-tan-1 values were being ignored when computing
+        btran_col, rather than averaging in a 1 value.)
+
+(3) Non-answer-changing fire code cleanup:
+
+    (a) Cleanup of the btran2 fixes, including reverting the TEMPORARY
+        CHANGE noted in (2b), instead relying on a better mechanism:
+        just doing the calculations of btran_col and wtlf over the
+        exposedvegp filter. Also, get rid of the checks for
+        shr_infnan_isnan(btran2(p)) and btran2(p) <= 1 (allowed by the
+        other changes in (2) and (3)).
+
+    (b) Set btran2 to 0 over non-exposed-veg points: this changes
+        answers for the BTRAN2 diagnostic field, but nothing else. (This
+        follows what is done for the standard BTRAN.)
+
+    (c) Move calc_fire_root_wetness for CNFireLi2021 into the base type
+        to avoid future bugs (assuming that the next fire module will
+        extend the base class but will also want to use this new version
+        of calc_fire_root_wetness).
+
+    (d) Change fire looping structure to be more standard
+
+(4) Remove some very expensive tests from aux_clm, putting some in the
+    new ctsm_sci test list instead
+
+(5) A bit of other minor cleanup
+
+Bugs fixed or introduced
+------------------------
+
+Issues fixed (include CTSM Issue #):
+- Resolves ESCOMP/CTSM#1139 (Decrease expense of ne0ARCTICGRISne30x8
+  test and C96 tests)
+- Resolves ESCOMP/CTSM#1153 (Fire btran2 is only computed for exposed
+  veg patches, but is used over all veg patches)
+- Resolves ESCOMP/CTSM#1170 (CNFire code: btran2 should not be skipped
+  when it's greater than 1)
+- Partially addresses ESCOMP/CTSM#1142 (Add ctsm_sci test list that
+  would be used for releases and making sure important resolutions run
+  well)
+
+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.]
+
+[X] clm5_1
+
+[X] clm5_0
+
+[X] ctsm5_0-nwp
+
+[X] 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): none
+
+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): none
+
+Changes to tests or testing: Remove some very expensive tests from
+aux_clm, putting some in the new ctsm_sci test list instead.
+
+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) - pass (on my mac)
+
+  regular tests (aux_clm):
+
+    cheyenne ---- ok
+    izumi ------- ok
+
+    ok: tests pass, many baselines fail as expected
+
+If the tag used for baseline comparisons was NOT the previous tag, note that here:
+
+
+Answer changes
+--------------
+
+Changes answers relative to baseline: YES, for all CN/BGC configurations
+
+  Summarize any changes to answers, i.e.,
+    - what code configurations: All CN/BGC configurations
+    - what platforms/compilers: all
+    - nature of change (roundoff; larger than roundoff/same climate; new climate): 
+      larger than roundoff; expected to be same climate, but not
+      investigated yet
+
+
+  To verify that the answer changes only came from expected changes, I
+  did a few rounds of testing. The following numbers refer to the groups
+  of changes listed under "Purpose of changes", above. The changes in
+  groups (4) and (5) were combined with those in groups (1) and (3) -
+  i.e., the basically non-answer-changing sets.
+
+  First, I tested with just the changes in (1). This led to differences
+  in only limited configurations, as noted in
+  https://github.com/ESCOMP/CTSM/pull/1155#issuecomment-695035048:
+  - nofire tests had diffs in BTRAN2
+  - dynroots tests had extensive diffs because now btran2 is calculated
+    after the dyn roots updates in each time step
+  - tests where the compared clm2 h0 file includes the 0th time step had
+    diffs just in BTRAN2, for reasons I couldn't determine
+
+  Then I tested with the changes in (2), which were expected to change
+  answers for all CN/BGC tests. In particular, these change answers due
+  to:
+  - using updated h2osoi_vol rather than the one earlier in the time
+    step when calculating btran2
+  - only considering points in the exposed veg filter when averaging
+    btran2 from patch to column, rather than using stale values from
+    no-longer-exposed patches (which also means having fire_m = 0 if
+    there are no currently-exposed veg patches in a column) (fix for
+    ESCOMP/CTSM#1153)
+  - treating btran2 values that are slightly greater than 1 as 1, rather
+    than ignoring them completely (fix for ESCOMP/CTSM#1170)
+
+  Finally, I tested with all of the changes, comparing against the
+  baselines generated from (2). As expected, the only answer changes in
+  this final round were in the BTRAN2 diagnostic field (due to setting
+  btran2 to 0 over non-exposed-veg points).
+
+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/1155
+
+===============================================================
+===============================================================
 Tag name:  ctsm5.1.dev006
 Originator(s):  sacks (Bill Sacks)
 Date:  Sat Oct  3 19:50:41 MDT 2020

diff --git a/doc/ChangeSum b/doc/ChangeSum
@@ -1,5 +1,6 @@
 Tag                   Who      Date  Summary
 ============================================================================================================================
+  ctsm5.1.dev007    sacks 10/06/2020 CNFire: btran2 fixes and general cleanup
   ctsm5.1.dev006    sacks 10/03/2020 Call correct routine to calculate btran2 for CNFireLi2021
   ctsm5.1.dev005    sacks 10/02/2020 Answer changing bug fixes for clm51: fire and organic_frac_squared
   ctsm5.1.dev004   oleson 09/30/2020 Improve robustness of onset and offset counters when changing dt

diff --git a/python/ctsm/run_sys_tests.py b/python/ctsm/run_sys_tests.py
@@ -164,7 +164,7 @@ def run_sys_tests(machine, cime_path,
         if not dry_run:
             _make_cs_status_non_suite(testroot, testid_base)
         if testfile:
-            test_args = ['--testfile', testfile]
+            test_args = ['--testfile', os.path.abspath(testfile)]
         elif testlist:
             test_args = testlist
         else:

diff --git a/src/biogeochem/CNDriverMod.F90 b/src/biogeochem/CNDriverMod.F90
@@ -40,6 +40,7 @@ module CNDriverMod
   use EnergyFluxType                  , only : energyflux_type
   use SaturatedExcessRunoffMod        , only : saturated_excess_runoff_type
   use ActiveLayerMod                  , only : active_layer_type
+  use SoilWaterRetentionCurveMod      , only : soil_water_retention_curve_type
   !
   ! !PUBLIC TYPES:
   implicit none
@@ -80,7 +81,8 @@ end subroutine CNDriverInit
 
   !-----------------------------------------------------------------------
   subroutine CNDriverNoLeaching(bounds,                                                    &
-       num_soilc, filter_soilc, num_soilp, filter_soilp, num_pcropp, filter_pcropp, doalb, &
+       num_soilc, filter_soilc, num_soilp, filter_soilp, num_pcropp, filter_pcropp,        &
+       num_exposedvegp, filter_exposedvegp, num_noexposedvegp, filter_noexposedvegp, doalb, &
        cnveg_state_inst,                                                                   &
        cnveg_carbonflux_inst, cnveg_carbonstate_inst,                                      &
        c13_cnveg_carbonflux_inst, c13_cnveg_carbonstate_inst,                              &
@@ -94,7 +96,8 @@ subroutine CNDriverNoLeaching(bounds,
        soilbiogeochem_nitrogenflux_inst, soilbiogeochem_nitrogenstate_inst,                &
        active_layer_inst,                                                                  &
        atm2lnd_inst, waterstatebulk_inst, waterdiagnosticbulk_inst, waterfluxbulk_inst,    &
-       wateratm2lndbulk_inst, canopystate_inst, soilstate_inst, temperature_inst, crop_inst, ch4_inst,            &
+       wateratm2lndbulk_inst, canopystate_inst, soilstate_inst, temperature_inst,          &
+       soil_water_retention_curve, crop_inst, ch4_inst,            &
        dgvs_inst, photosyns_inst, saturated_excess_runoff_inst, energyflux_inst,                   &
        nutrient_competition_method, cnfire_method, dribble_crophrv_xsmrpool_2atm)
     !
@@ -146,6 +149,10 @@ subroutine CNDriverNoLeaching(bounds,
     integer                                 , intent(in)    :: filter_soilp(:)   ! filter for soil patches
     integer                                 , intent(in)    :: num_pcropp        ! number of prog. crop patches in filter
     integer                                 , intent(in)    :: filter_pcropp(:)  ! filter for prognostic crop patches
+    integer                                 , intent(in)    :: num_exposedvegp        ! number of points in filter_exposedvegp
+    integer                                 , intent(in)    :: filter_exposedvegp(:)  ! patch filter for non-snow-covered veg
+    integer                                 , intent(in)    :: num_noexposedvegp       ! number of points in filter_noexposedvegp
+    integer                                 , intent(in)    :: filter_noexposedvegp(:) ! patch filter where frac_veg_nosno is 0 
     logical                                 , intent(in)    :: doalb             ! true = surface albedo calculation time step
     type(cnveg_state_type)                  , intent(inout) :: cnveg_state_inst
     type(cnveg_carbonflux_type)             , intent(inout) :: cnveg_carbonflux_inst
@@ -178,6 +185,7 @@ subroutine CNDriverNoLeaching(bounds,
     type(canopystate_type)                  , intent(inout)    :: canopystate_inst
     type(soilstate_type)                    , intent(inout) :: soilstate_inst
     type(temperature_type)                  , intent(inout) :: temperature_inst
+    class(soil_water_retention_curve_type)  , intent(in)    :: soil_water_retention_curve
     type(crop_type)                         , intent(inout) :: crop_inst
     type(ch4_type)                          , intent(in)    :: ch4_inst
     type(dgvs_type)                         , intent(inout) :: dgvs_inst
@@ -747,7 +755,9 @@ subroutine CNDriverNoLeaching(bounds,
 
     call t_startf('CNFire')
     call cnfire_method%CNFireArea(bounds, num_soilc, filter_soilc, num_soilp, filter_soilp, &
+         num_exposedvegp, filter_exposedvegp, num_noexposedvegp, filter_noexposedvegp, &
          atm2lnd_inst, energyflux_inst, saturated_excess_runoff_inst, waterdiagnosticbulk_inst, wateratm2lndbulk_inst, &
+         waterstatebulk_inst, soilstate_inst, soil_water_retention_curve, &
          cnveg_state_inst, cnveg_carbonstate_inst, &
          totlitc_col=soilbiogeochem_carbonstate_inst%totlitc_col(begc:endc), &
          decomp_cpools_vr_col=soilbiogeochem_carbonstate_inst%decomp_cpools_vr_col(begc:endc,1:nlevdecomp_full,1:ndecomp_pools), &