Merge remote-tracking branch 'escomp/master' into remove_clm4

.gitignore

@@ -24,6 +24,8 @@ manage_externals.log
 build/
 CMakeFiles/
 
+# symbolic links generated by run_sys_tests
+/tests_*
 
 # unit testing directories
 /src/unit_tests.*

doc/ChangeLog

@@ -1,4 +1,200 @@
 ===============================================================
+Tag name:  ctsm1.0.dev022
+Originator(s):  sacks (Bill Sacks)
+Date: Tue Jan  8 11:01:38 MST 2019
+One-line Summary: Set tracer version of irrigation fluxes
+
+Purpose of changes
+------------------
+
+Set tracer version of irrigation fluxes.
+
+This required a substantial rewrite of ApplyIrrigation (now renamed to
+CalcIrrigationFluxes). The problem was that the code was written to
+compute the total irrigation withdrawal from groundwater, then use this
+to compute the application fluxes (drip/sprinkler), then later divide
+this total withdrawal by layer. But for tracer fluxes to be computed
+correctly, we needed to reorder this, so that the per-layer withdrawals
+are determined before determining the application fluxes. This is
+because the application flux needs to know the tracer concentrations of
+the source, which requires knowing which layers the source is drawing
+from. This was made more challenging because of the mix of patch-level
+and column-level variables at play: irrigation demand is patch-level,
+groundwater availability and extraction is column-level, but application
+is back to patch-level.
+
+In a somewhat-related change, I also reworked the passing of information
+between soil hydrology (groundwater availability) and irrigation:
+Previously, there was some near-duplicate code in
+CalcAvailableUnconfinedAquifer (which was called prior to
+ApplyIrrigation) and WithdrawGroundwaterIrrigation (which was called
+after ApplyIrrigation). I have reworked the code to remove this
+duplication, calling the new CalcIrrigWithdrawals in the midst of
+CalcIrrigationFluxes. In doing so, I reconciled an accidental
+discrepancy between the two original routines (see
+https://github.com/escomp/ctsm/issues/595).
+
+
+Bugs fixed or introduced
+------------------------
+
+Issues fixed (include CTSM Issue #):
+- Resolves ESCOMP/ctsm#521 (Set tracer versions of fluxes set by
+  ApplyIrrigation)
+- Resolves ESCOMP/ctsm#593 (Generalize groundwater irrigation availability to
+  handle multiple patches per column)
+- Resolves ESCOMP/ctsm#595 (Inconsistency between CalcAvailableUnconfinedAquifer
+  and WithdrawGroundwaterIrrigation)
+
+
+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_0
+
+[ ] clm4_5
+
+[ ] clm4_0
+
+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)
+---------------------------------------------
+
+Caveats for developers (e.g., code that is duplicated that requires double maintenance): none
+
+Changes to tests or testing: none
+
+Code reviewed by:
+
+- Sean Swenson reviewed the initial parts of the rework of
+  ApplyIrrigation and the passing of information between soil hydrology
+  and irrigation
+
+CTSM testing:
+
+  build-namelist tests:
+
+    cheyenne - not run
+
+  tools-tests (test/tools):
+
+    cheyenne - not run
+
+  PTCLM testing (tools/shared/PTCLM/test):
+
+     cheyenne - not run
+
+  regular tests (aux_clm):
+
+    cheyenne ---- ok
+    hobart ------ ok
+
+    ok means tests pass, expected baseline failures as noted below
+
+  Additional manual testing:
+
+  - Out of the box, the one-timestep tracer consistency test
+    (SMS_D_Ln1.f10_f10_musgs.I2000Clm50BgcCropGs.hobart_nag.clm-tracer_consistency)
+    passed when the new check was inserted after the irrigation call,
+    even without the changes in this tag. Presumably this is because no
+    irrigation was being done in the first time step of this test. To
+    confirm that the changes here were both necessary and effective, I
+    hacked the code to force irrigation of all types (surface and
+    groundwater) on the first time step (see
+    https://github.com/billsacks/ctsm/commit/d28a03145). I confirmed
+    that this test, with those code hacks, failed before the changes in
+    this tag, and passed with these changes in place.
+
+  - I used a multi-step process to confirm that these changes are only
+    roundoff-level different for groundwater irrigation (because
+    feedbacks in the system result in
+    SMS_D_Ld5.f10_f10_musgs.I2000Clm50BgcCrop.hobart_nag.clm-irrig_alternate
+    looking greater-than-roundoff-level different from the baseline):
+
+    (1) Confirmed that e507fe603 is only roundoff-level different from
+        baseline using cprnc output from this test.
+
+    (2) Confirmed that a4a1a626c is only roundoff-level different from
+        e507fe603. This was the tricky part. I did this by introducing
+        some temporary code that (a) computed some fluxes in both the
+        old and new ways, (b) compared the old and new methods for each
+        point and time step, confirming that they were no more than
+        roundoff-level different, then (c) set the fluxes to the old
+        method. I confirmed that this was bit-for-bit with e507fe603,
+        and the checks for greater-than-roundoff-level differences
+        between the old and new methods were never triggered. (See
+        https://github.com/billsacks/ctsm/commit/7d23e9e and the earlier
+        commits on that branch.)
+
+    (3) Confirmed that remaining changes an the branch are bit-for-bit
+        with a4a1a626c.
+
+CTSM tag used for the baseline comparisons: ctsm1.0.dev021
+
+
+Answer changes
+--------------
+
+Changes answers relative to baseline: YES
+
+  If a tag changes answers relative to baseline comparison the
+  following should be filled in (otherwise remove this section):
+
+  Summarize any changes to answers, i.e.,
+    - what code configurations:
+
+      - Small answer changes when groundwater irrigation is enabled. In
+        principle, could change answers by greater than roundoff due to
+        fix of #595, but I didn't see any answer changes in my 7-month
+        test due to that change. Other than that, just roundoff-level
+        changes.
+
+      - Answer changes when tracers are enabled.
+
+      - Roundoff-level changes in the diagnostic field,
+        QIRRIG_FROM_SURFACE, for many tests
+
+    - what platforms/compilers: all
+    - nature of change (roundoff; larger than roundoff/same climate; new climate): 
+      See notes under "what code configurations" for details.
+
+   If bitwise differences were observed, how did you show they were no worse
+   than roundoff? N/A
+
+   If this tag changes climate describe the run(s) done to evaluate the new
+   climate (put details of the simulations in the experiment database)
+       - casename: N/A
+
+   URL for LMWG diagnostics output used to validate new climate: N/A
+
+
+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/600
+
+===============================================================
+===============================================================
 Tag name:  ctsm1.0.dev021
 Originator(s):  mvr (Mathew Rothstein,UCAR/CSEG,303-497-1304)
 Date: Wed Dec 26 16:29:06 MST 2018

doc/ChangeSum

@@ -1,5 +1,6 @@
 Tag                   Who      Date  Summary
 ============================================================================================================================
+  ctsm1.0.dev022    sacks 01/08/2019 Set tracer version of irrigation fluxes
   ctsm1.0.dev021      mvr 12/26/2018 Added tracer ratio capability and included it in consistency checks
   ctsm1.0.dev020    sacks 12/03/2018 New options for irrigation and crop fsat
   ctsm1.0.dev019    sacks 11/30/2018 Rework cold start initialization of wa and zwt

python/ctsm/joblauncher/job_launcher_no_batch.py

@@ -19,6 +19,7 @@ def __init__(self, nice_level=None):
         nice_level: Level used for the nice command (larger = lower priority, up to 19)
         """
         JobLauncherBase.__init__(self)
+        self._process = None
         if nice_level is None:
             # We have a default value of None rather than 0 in the argument list because
             # objects of this class may be created via a few layers, and we want to allow
@@ -46,10 +47,10 @@ def run_command_impl(self, command, stdout_path, stderr_path):
             # Note that preexec_fn is POSIX-only; also, it may be unsafe in the presence
             # of threads (hence the need for disabling the pylint warning)
             # pylint: disable=subprocess-popen-preexec-fn
-            subprocess.Popen(command,
-                             stdout=outfile,
-                             stderr=errfile,
-                             preexec_fn=self._preexec)
+            self._process = subprocess.Popen(command,
+                                             stdout=outfile,
+                                             stderr=errfile,
+                                             preexec_fn=self._preexec)
 
     def run_command_logger_message(self, command, stdout_path, stderr_path):
         message = 'Running: <{command_str}> ' \
@@ -58,3 +59,8 @@ def run_command_logger_message(self, command, stdout_path, stderr_path):
                                                   outfile=stdout_path,
                                                   errfile=stderr_path)
         return message
+
+    def wait_for_last_process_to_complete(self):
+        """Waits for the last process started by run_command_impl (if any) to complete"""
+        if self._process is not None:
+            self._process.wait()

python/ctsm/run_sys_tests.py

@@ -4,12 +4,14 @@
 import argparse
 import logging
 import os
+import subprocess
 from datetime import datetime
 
 from ctsm.ctsm_logging import setup_logging_pre_config, add_logging_args, process_logging_args
 from ctsm.machine_utils import get_machine_name, make_link
 from ctsm.machine import create_machine
 from ctsm.machine_defaults import MACHINE_DEFAULTS
+from ctsm.path_utils import path_to_ctsm_root
 
 from CIME.test_utils import get_tests_from_xml  # pylint: disable=import-error
 from CIME.cs_status_creator import create_cs_status  # pylint: disable=import-error
@@ -52,7 +54,9 @@ def main(cime_path):
     logger.debug("Machine info: %s", machine)
 
     run_sys_tests(machine=machine, cime_path=cime_path,
-                  skip_testroot_creation=args.skip_testroot_creation, dry_run=args.dry_run,
+                  skip_testroot_creation=args.skip_testroot_creation,
+                  skip_git_status=args.skip_git_status,
+                  dry_run=args.dry_run,
                   suite_name=args.suite_name, testfile=args.testfile, testlist=args.testname,
                   suite_compilers=args.suite_compiler,
                   testid_base=args.testid_base, testroot_base=args.testroot_base,
@@ -62,7 +66,9 @@ def main(cime_path):
                   extra_create_test_args=args.extra_create_test_args)
 
 def run_sys_tests(machine, cime_path,
-                  skip_testroot_creation=False, dry_run=False,
+                  skip_testroot_creation=False,
+                  skip_git_status=False,
+                  dry_run=False,
                   suite_name=None, testfile=None, testlist=None,
                   suite_compilers=None,
                   testid_base=None, testroot_base=None,
@@ -79,6 +85,7 @@ def run_sys_tests(machine, cime_path,
     cime_path (str): path to root of cime
     skip_testroot_creation (bool): if True, assume the testroot directory has already been
         created, so don't try to recreate it or re-make the link to it
+    skip_git_status (bool): if True, skip printing git and manage_externals status
     dry_run (bool): if True, print commands to be run but don't run them
     suite_name (str): name of test suite/category to run
     testfile (str): path to file containing list of tests to run
@@ -115,6 +122,8 @@ def run_sys_tests(machine, cime_path,
     testroot = _get_testroot(testroot_base, testid_base)
     if not skip_testroot_creation:
         _make_testroot(testroot, testid_base, dry_run)
+    if not skip_git_status:
+        _record_git_status(testroot, dry_run)
     print("Testroot: {}".format(testroot))
 
     create_test_args = _get_create_test_args(compare_name=compare_name,
@@ -286,6 +295,13 @@ def _commandline_args():
                         help='Do not create the directory that will hold the tests.\n'
                         'This should be used if the desired testroot directory already exists.')
 
+    parser.add_argument('--skip-git-status', action='store_true',
+                        help='Skip printing git and manage_externals status,\n'
+                        'both to screen and to the SRCROOT_GIT_STATUS file in TESTROOT.\n'
+                        'This printing can often be helpful, but this option can be used to\n'
+                        'avoid extraneous output, to reduce the time needed to run this script,\n'
+                        'or if git or manage_externals are currently broken in your sandbox.\n')
+
     parser.add_argument('--dry-run', action='store_true',
                         help='Print what would happen, but do not run any commands.\n'
                         '(Generally should be run with --verbose.)\n')
@@ -340,6 +356,39 @@ def _make_testroot(testroot, testid_base, dry_run):
         os.makedirs(testroot)
         make_link(testroot, _get_testdir_name(testid_base))
 
+def _record_git_status(testroot, dry_run):
+    """Record git status and related information to stdout and a file"""
+    output = ''
+    ctsm_root = path_to_ctsm_root()
+
+    current_hash = subprocess.check_output(['git', 'rev-parse', 'HEAD'],
+                                           cwd=ctsm_root,
+                                           universal_newlines=True)
+    output += "Current hash: {}".format(current_hash)
+    git_status = subprocess.check_output(['git', '-c', 'color.ui=always',
+                                          'status', '--short', '--branch'],
+                                         cwd=ctsm_root,
+                                         universal_newlines=True)
+    output += git_status
+    if git_status.count('\n') == 1:
+        # Only line in git status is the branch info
+        output += "(clean sandbox)\n"
+    manic = os.path.join('manage_externals', 'checkout_externals')
+    manage_externals_status = subprocess.check_output([manic, '--status', '--verbose'],
+                                                      cwd=ctsm_root,
+                                                      universal_newlines=True)
+    output += 72*'-' + '\n' + 'manage_externals status:' + '\n'
+    output += manage_externals_status
+    output += 72*'-' + '\n'
+
+    print(output)
+
+    if not dry_run:
+        git_status_filepath = os.path.join(testroot, 'SRCROOT_GIT_STATUS')
+        with open(git_status_filepath, 'w') as git_status_file:
+            git_status_file.write("SRCROOT: {}\n".format(ctsm_root))
+            git_status_file.write(output)
+
 def _get_create_test_args(compare_name, generate_name, baseline_root,
                           account, walltime, queue,
                           extra_create_test_args):

python/ctsm/test/joblauncher/test_job_launcher_no_batch.py

@@ -39,6 +39,7 @@ def test_runCommand(self):
         job_launcher.run_command(command=['echo', 'hello', 'world'],
                                  stdout_path=stdout,
                                  stderr_path=os.path.join(self._testdir, 'stderr'))
+        job_launcher.wait_for_last_process_to_complete()
         self.assertTrue(os.path.isfile(stdout))
         self.assertFileContentsEqual('hello world\n', stdout)
 
@@ -49,4 +50,10 @@ def test_runCommand_dryRun(self):
                                  stdout_path=os.path.join(self._testdir, 'stdout'),
                                  stderr_path=os.path.join(self._testdir, 'stderr'),
                                  dry_run=True)
+        # There shouldn't be a "last process", but in case there is, wait for it to
+        # complete so we can be confident that the test isn't passing simply because the
+        # process hasn't completed yet. (This relies on there being logic in
+        # wait_for_last_process_to_complete so that it succeeds even if there is no "last
+        # process".)
+        job_launcher.wait_for_last_process_to_complete()
         self.assertEqual(os.listdir(self._testdir), [])

python/ctsm/test/test_run_sys_tests.py

@@ -25,6 +25,8 @@
 # to make readable unit test names
 # pylint: disable=invalid-name
 
+# Replace the slow _record_git_status with a fake that does nothing
+@mock.patch('ctsm.run_sys_tests._record_git_status', mock.MagicMock(return_value=None))
 class TestRunSysTests(unittest.TestCase):
     """Tests of run_sys_tests"""
 
@@ -116,7 +118,7 @@ def test_createTestCommand_testnames(self):
         six.assertRegex(self, command, r'--test-id +{}\s'.format(self._expected_testid()))
         expected_testroot_path = os.path.join(self._scratch, self._expected_testroot())
         six.assertRegex(self, command, r'--test-root +{}\s'.format(expected_testroot_path))
-        six.assertRegex(self, command, r'test1 +test2 *$')
+        six.assertRegex(self, command, r'test1 +test2(\s|$)')
         assertNotRegex(self, command, r'--compare\s')
         assertNotRegex(self, command, r'--generate\s')