address one wave of review comments

.gitmodules

@@ -7,7 +7,7 @@
 [submodule "history"]
 	path = src/history/buffers
 	url = https://github.com/peverwhee/history_output
-	fxtag         = sima-history
+	fxtag         = 44099b18b2dcbb80c1ed1755b6ef1cc7be028033
 	fxrequired    = AlwaysRequired
 	fxDONOTUSEurl = https://github.com/peverwhee/history_output
 [submodule "mpas"]
@@ -20,7 +20,7 @@
 [submodule "ncar-physics"]
         path          = src/physics/ncar_ccpp
 	url           = https://github.com/peverwhee/atmospheric_physics
-	fxtag         = diagnostics
+	fxtag         = 61bd9d3dac2abb11bb1e44a2ca34b401da0f44b1
         fxrequired    = AlwaysRequired
         fxDONOTUSEurl = https://github.com/ESCOMP/atmospheric_physics
 [submodule "ccs_config"]

cime_config/buildnml

@@ -307,26 +307,6 @@ def buildnml(case, caseroot, compname):
         user_nl_fname = "user_nl_cam" + inst_string
         user_nl_file = os.path.join(caseroot, user_nl_fname)
 
-        # Temporary user_nl file with history config stripped
-        user_nl_temp = os.path.join(confdir, "user_nl_temp")
-        if os.path.exists(user_nl_temp):
-            os.remove(user_nl_temp)
-        # end if
-
-        # Remove history configuration from the normal user_nl content
-        with open(user_nl_file, 'r') as infile:
-            clines = infile.readlines()
-        # end with
-        with open(user_nl_temp, 'w') as outfile:
-            for line in clines:
-                sline = line.strip()
-                if ((not sline) or (sline[0] == '!') or
-                    (_USER_NL_LINE.match(sline) is not None)):
-                    outfile.write(line)
-                # end if
-            # end or
-        # end with
-
         # Check that file actually exists.  If not then throw an error:
         if not os.path.exists(user_nl_file):
             emsg = "The file 'user_nl_cam' is missing.  Please run 'case.setup' first."

src/data/registry.xml

@@ -203,15 +203,15 @@
       <ic_file_input_names>zi state_zi</ic_file_input_names>
     </variable>
     <variable local_name="te_ini"
-              standard_name="vertically_integrated_energies_of_initial_state_in_cam"
+              standard_name="vertically_integrated_total_energy_of_initial_state"
               units="J m-2"
               type="real" kind="kind_phys"
               allocatable="pointer">
       <dimensions>horizontal_dimension</dimensions>
       <ic_file_input_names>te_ini state_te_ini</ic_file_input_names>
     </variable>
     <variable local_name="te_cur"
-              standard_name="vertically_integrated_energies_of_current_state_in_cam"
+              standard_name="vertically_integrated_total_energy_of_current_state"
               units="J m-2"
               type="real" kind="kind_phys"
               allocatable="pointer">
@@ -297,8 +297,8 @@
       <data>inverse_exner_function_wrt_surface_pressure</data>
       <data>frontogenesis_function</data>
       <data>frontogenesis_angle</data>
-      <data>vertically_integrated_energies_of_initial_state_in_cam</data>
-      <data>vertically_integrated_energies_of_current_state_in_cam</data>
+      <data>vertically_integrated_total_energy_of_initial_state</data>
+      <data>vertically_integrated_total_energy_of_current_state</data>
       <data>vertically_integrated_total_water_of_initial_state</data>
       <data>vertically_integrated_total_water_of_current_state</data>
     </ddt>

test/unit/sample_files/hist_config_files/amwg_hist_config

@@ -1,7 +1,6 @@
 hist_max_frames: 1
-hist_output_frequency: monthly
+hist_output_frequency: 1*months
 hist_precision: REAL32
-!hist_output_levels: IPCC_PRESSURE_LEVELS
 
 ! ADF mean
 !hist_diag_file: adf_mean_config

test/unit/sample_files/hist_config_files/atm_in_multi

@@ -53,7 +53,7 @@
                     'FLDS_d4    ', 'FLDS_d5    ', 'FLDS_d6    ', 'FLDS_d7    ', 'FLDS_d8    ', 'FLDS_d9    ',
                     'FLDS_d10   ', 'AODDUST1   ', 'AODDUST3   ', 'AODDUST    ', 'AODVIS     ', 'CCN3       '
     hist_max_frames = 1
-    hist_output_frequency = '1*monthly'
+    hist_output_frequency = '1*nmonths'
     hist_precision = 'REAL32'
     hist_file_type = 'history'
     hist_filename_spec = '%c.cam.%u.%y-%m-%d-%s.nc'
@@ -67,6 +67,6 @@
     hist_output_frequency = '2*nsteps'
     hist_precision = 'REAL64'
     hist_file_type = 'history'
-    hist_filename_spec = '%c.cam.%u.%y-%m-%d-%s.nc'
+    hist_filename_spec = 'test_fname_%y.nc'
     hist_write_nstep0 = .true.
 /

test/unit/sample_files/hist_config_files/user_nl_cam_multi

@@ -8,11 +8,12 @@ use_topo_file = .false.
 hist_no_defaults: True
 hist_diag_file;h0: amwg_hist_config
 hist_remove_fields;h0: TAUTMSX, TAUTMSY
-hist_output_frequency;h0: monthly
+hist_output_frequency;h0: 1*nmonths
 hist_precision;h0: REAL32
 
 hist_add_inst_fields;h3: T, U, V
 hist_output_frequency;h3: 2*nsteps
 hist_precision;h3: REAL64
 hist_max_frames;h3: 24
-hist_write_nstep0;h3 = .true.
+hist_write_nstep0;h3: .true.
+hist_filename_template;h3: test_fname_%y.nc

test/unit/test_cam_autogen.py

@@ -158,12 +158,6 @@ def ic_names(self):
 
         return {}
 
-    def diag_names(self):
-
-        """Fake version of 'diag_names' property."""
-
-        return {}
-
     # pylint: enable=no-self-use
     # pylint: enable=unused-argument
 

test/unit/test_hist_config.py

@@ -28,15 +28,16 @@
 _LOGGER = logging.getLogger(__name__)
 
 if not os.path.exists(__CIME_CONFIG_DIR):
-    raise ImportError("Cannot find <root>/cime_config")
+    raise ImportError(f"Cannot find '{__CIME_CONFIG_DIR}'")
 
 if not os.path.exists(_SAMPLE_FILES_DIR):
-    raise ImportError("Cannot find sample files directory")
+    raise ImportError(f"Cannot find '{_SAMPLE_FILES_DIR}'")
 
 sys.path.append(__CIME_CONFIG_DIR)
 
 # pylint: disable=wrong-import-position
 from hist_config import HistoryConfig
+from hist_config import HistoryConfigError
 # pylint: enable=wrong-import-position
 
 ###############################################################################
@@ -62,19 +63,26 @@ def setUpClass(cls):
         remove_files(glob.iglob(os.path.join(_TMP_DIR, '*.*')))
         super(cls, HistConfigTest).setUpClass()
 
-    def _test_config(self, config, vol, prec, maxf, outfreq, ftype):
+    def _test_config(self, config, vol, prec, maxf, outfreq, ftype, write_nstep0, filename_spec, restart_fname_spec):
         """Check the properties of <config> against the other inputs:
         <vol>: volume
         <prec>: precision
         <maxf>: max_frames
         <outfreq>: output_frequency
-        <ftype>: file_type"""
+        <ftype>: file_type
+        <write_nstep0>: flag to write the 0th timestep
+        <filename_spec>: filename template
+        <restart_fname_spec>: restart filename template
+        """
         self.assertEqual(config.volume, vol, msg="Bad volume")
         self.assertEqual(config.precision, prec, msg="Bad precision")
         self.assertEqual(config.max_frames, maxf, msg="Bad max frames")
         self.assertEqual(config.output_frequency, outfreq,
                          msg="Bad output frequency")
         self.assertEqual(config.file_type, ftype, msg="Bad file type")
+        self.assertEqual(config.write_nstep0, write_nstep0, msg="Bad write_nste0 flag")
+        self.assertEqual(config.filename_spec, filename_spec, msg="Bad filename spec")
+        self.assertEqual(config.restart_fname_spec, restart_fname_spec, msg="Bad restart filename spec")
 
     def test_flat_user_nl_cam(self):
         """Test history entries that would be appropriate in user_nl_cam.
@@ -85,8 +93,16 @@ def test_flat_user_nl_cam(self):
         out_test = os.path.join(_SAMPLE_FILES_DIR, "atm_in_flat")
         remove_files([out_source])
         # Run test
-        _LOGGER.setLevel(logging.DEBUG)
-        hist_configs = HistoryConfig(filename=in_source, logger=_LOGGER)
+        hist_log = logging.getLogger("hist_log")
+        #_LOGGER.setLevel(logging.DEBUG)
+        with self.assertLogs(hist_log, level='DEBUG') as cmplog:
+            hist_configs = HistoryConfig(filename=in_source, logger=hist_log)
+        # end with
+        # Check that the first few lines of the log are as expected
+        expected_logmsg = ["DEBUG:hist_log:Added average field, 'MOE' to hist volume, h1, at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/sample_files/hist_config_files/user_nl_cam_flat:5",
+                           "DEBUG:hist_log:Added average field, 'LARRY' to hist volume, h1, at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/sample_files/hist_config_files/user_nl_cam_flat:5",
+                           "DEBUG:hist_log:Added average field, 'CURLY' to hist volume, h1, at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/sample_files/hist_config_files/user_nl_cam_flat:5"]
+        self.assertEqual(cmplog.output[0:3], expected_logmsg)
         # Check that HistoryConfig object was created
         amsg = "Test failure: no HistConfig object created"
         self.assertTrue(isinstance(hist_configs, HistoryConfig), msg=amsg)
@@ -96,10 +112,11 @@ def test_flat_user_nl_cam(self):
         # Check properties of created config objects
         self.assertTrue('h1' in hist_configs, msg="'h1' not in hist_configs")
         hconfig = hist_configs['h1']
-        self._test_config(hconfig, 'h1', 'REAL32', 30, (14, 'hours'), 'history')
+        self._test_config(hconfig, 'h1', 'REAL32', 30, (14, 'hours'), 'history', '.false.', '%c.cam.%u.%y-%m-%d-%s.nc', '%c.cam.r%u.%y-%m-%d-%s.nc')
         self.assertTrue('h3' in hist_configs, msg="'h3' not in hist_configs")
         hconfig = hist_configs['h3']
-        self._test_config(hconfig, 'h3', 'REAL64', 24, (2, 'nsteps'), 'history')
+        self._test_config(hconfig, 'h3', 'REAL64', 24, (2, 'nsteps'), 'history', '.false.', '%c.cam.%u.%y-%m-%d-%s.nc', '%c.cam.r%u.%y-%m-%d-%s.nc')
+        _LOGGER.setLevel(logging.DEBUG)
         # Write out the namelist file
         with open(out_source, 'w', encoding='utf-8') as nl_file:
             hist_configs.output_class_namelist(nl_file)
@@ -112,7 +129,7 @@ def test_flat_user_nl_cam(self):
         self.assertTrue(os.path.exists(out_source), msg=amsg)
         # Make sure the output file is correct
         amsg = f"{out_source} does not match {out_test}"
-        self.assertTrue(filecmp.cmp(out_test, out_source, shallow=False),
+        self.assertTrue(filecmp.cmp(out_test, out_source, shallow='.false.'),
                         msg=amsg)
 
     def test_multi_user_nl_cam(self):
@@ -136,10 +153,10 @@ def test_multi_user_nl_cam(self):
         # Check properties of created config objects
         self.assertTrue('h0' in hist_configs, msg="'h0' not in hist_configs")
         hconfig = hist_configs['h0']
-        self._test_config(hconfig, 'h0', 'REAL32', 1, (1, 'monthly'), 'history')
+        self._test_config(hconfig, 'h0', 'REAL32', 1, (1, 'nmonths'), 'history', '.false.', '%c.cam.%u.%y-%m-%d-%s.nc', '%c.cam.r%u.%y-%m-%d-%s.nc')
         self.assertTrue('h3' in hist_configs, msg="'h3' not in hist_configs")
         hconfig = hist_configs['h3']
-        self._test_config(hconfig, 'h3', 'REAL64', 24, (2, 'nsteps'), 'history')
+        self._test_config(hconfig, 'h3', 'REAL64', 24, (2, 'nsteps'), 'history', '.true.', 'test_fname_%y.nc', '%c.cam.r%u.%y-%m-%d-%s.nc')
         # Write out the namelist file
         with open(out_source, 'w', encoding='utf-8') as nl_file:
             hist_configs.output_class_namelist(nl_file)
@@ -152,9 +169,56 @@ def test_multi_user_nl_cam(self):
         self.assertTrue(os.path.exists(out_source), msg=amsg)
         # Make sure the output file is correct
         amsg = f"{out_source} does not match {out_test}"
-        self.assertTrue(filecmp.cmp(out_test, out_source, shallow=False),
+        self.assertTrue(filecmp.cmp(out_test, out_source, shallow='.false.'),
                         msg=amsg)
 
+    def test_bad_user_nl_cam(self):
+        """Test invalid history entries; confirm correct errors are thrown"""
+        # Setup test
+        in_source = os.path.join(_SAMPLE_FILES_DIR, "user_nl_cam_multi")
+        modified_in_source = os.path.join(_TMP_DIR, "user_nl_cam_multi_bad")
+        out_source = os.path.join(_TMP_DIR, "atm_in_multi")
+        out_test = os.path.join(_SAMPLE_FILES_DIR, "atm_in_multi")
+        remove_files([out_source])
+
+        # Open good user_nl_cam from previous test
+        with open(in_source, "r", encoding="utf-8") as old_file:
+            # Read in file:
+            file_lines = old_file.readlines()
+            # Edit to add bad lines
+            print(file_lines[14])
+            file_lines[8] = ""
+            file_lines[9] = "hist_remove_fields;h0:\n"
+            file_lines[10] = "hist_output_frequency;h0: 1+nmonths\n"
+            file_lines[11] = "hist_precision;h0: REAL34\n"
+            file_lines[13] = "hist_add_inst_fields;h3: T&U&V\n"
+            file_lines[16] = "hist_max_frames;h3: -24\n"
+            file_lines[17] = "hist_write_nstep0;h3: treu\n"
+        # end with
+
+        # Create a new modified version of the file with the bad entries
+        with open(modified_in_source, "w", encoding="utf-8") as new_file:
+            # Write lines to new file
+            new_file.writelines(file_lines)
+        # end with
+
+        # Run test
+        with self.assertRaises(HistoryConfigError) as err:
+            hist_configs = HistoryConfig(filename=modified_in_source, logger=_LOGGER)
+        # end with
+
+        self.maxDiff = None
+
+        # Check that the error message matches what is expected:
+        errmsg = "No identifiers found, at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/tmp/user_nl_cam_multi_bad:9\n"  \
+                  "period must be one of nsteps, nstep, nseconds, nsecond, nminutes, nminute, nhours, nhour, ndays, nday, nmonths, nmonth, nyears, nyear, steps, seconds, minutes, hours, days, months, years, at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/tmp/user_nl_cam_multi_bad:10\n" \
+                  "precision must be one of REAL32, REAL64, at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/tmp/user_nl_cam_multi_bad:11\n"  \
+                  "'T&U&V' is not a valid identifier, at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/tmp/user_nl_cam_multi_bad:13\n"  \
+                  "Attempt to set max frames to '-24', must be a positive integer, at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/tmp/user_nl_cam_multi_bad:16\n"  \
+                  "hist_write_nstep0 must be one of true, t, .true., false, f, .false., at /glade/u/home/courtneyp/Projects/sima-history-clean/test/unit/tmp/user_nl_cam_multi_bad:17"
+        self.assertEqual(errmsg, str(err.exception))
+
+
 ##############################################################################
 
 if __name__ == '__main__':