address review comments

cime_config/hist_config.py

@@ -44,7 +44,9 @@
 _OUT_PRECS = ['REAL32', 'REAL64']
 _TRUE_VALUES  = {"true", "t", ".true."}
 _FALSE_VALUES = {"false", "f", ".false."}
-_NETCDF_ID_RE = re.compile(r"^[a-z]\w{0,62}$", re.IGNORECASE)
+# NetCDF variable name requirements:
+#  https://docs.unidata.ucar.edu/netcdf-c/current/programming_notes.html#object_name
+_NETCDF_ID_RE = re.compile(r"^[a-z][\w._@+]{0,256}$", re.IGNORECASE)
 
 ##############################################################################
 ###
@@ -125,22 +127,20 @@ def _list_of_idents(entry, sep=','):
     (['foo', 'BA2r3'], None)
     >>> _list_of_idents("foo, 3bar")
     (None, 'Found invalid identifiers:\\n    3bar')
-    >>> _list_of_idents("foo.3bar")
-    (None, 'Found invalid identifiers:\\n    foo.3bar')
-    >>> _list_of_idents("foo3bariendnaadfasdfbasdlkfap983rasdfvalsda938qjnasdasd98adfasxd")
-    (None, 'Found invalid identifiers:\\n    foo3bariendnaadfasdfbasdlkfap983rasdfvalsda938qjnasdasd98adfasxd')
-    >>> _list_of_idents("foo.3bar, foo3bariendnaadfasdfbasdlkfap983rasdfvalsda938qjnasdasd98adfasxd")
-    (None, 'Found invalid identifiers:\\n    foo.3bar\\n    foo3bariendnaadfasdfbasdlkfap983rasdfvalsda938qjnasdasd98adfasxd')
-    >>> _list_of_idents("foo.3bar; foo", sep=';')
-    (None, 'Found invalid identifiers:\\n    foo.3bar')
+    >>> _list_of_idents("foo,3bar", sep=';')
+    (None, 'Found invalid identifiers:\\n    foo,3bar')
+    >>> _list_of_idents("foo#3bar, foo3baifijeowfjeiofjewiofjeiwofjewiofejifwjoefdfewfefdfdkjokmcdioanicdiaoilfejieojwiefjidojfioejsiofdjkljnxpoiadjfioenskcodiafkamd199fd9a0fdjkldajfdfjiodanckdalirhgieoskjcdskdfieowfidjfslk129dkjfaiocsriendnaadfasdfbasdlkfap983rasdfvalsda938qjnasdasd98adfasxd")
+    (None, 'Found invalid identifiers:\\n    foo#3bar\\n    foo3baifijeowfjeiofjewiofjeiwofjewiofejifwjoefdfewfefdfdkjokmcdioanicdiaoilfejieojwiefjidojfioejsiofdjkljnxpoiadjfioenskcodiafkamd199fd9a0fdjkldajfdfjiodanckdalirhgieoskjcdskdfieowfidjfslk129dkjfaiocsriendnaadfasdfbasdlkfap983rasdfvalsda938qjnasdasd98adfasxd')
+    >>> _list_of_idents("foo,3bar; foo", sep=';')
+    (None, 'Found invalid identifiers:\\n    foo,3bar')
     >>> _list_of_idents(" ")
     (None, 'No identifiers found')
     """
     if entry.strip():
         potential_list = [x.strip() for x in entry.split(sep)]
         bad_list = [sample for sample in potential_list if _NETCDF_ID_RE.match(sample) is None]
         if len(bad_list) > 0:
-            return (None, f"Found invalid identifiers:\n    " + '\n    '.join(bad_list))
+            return (None, "Found invalid identifiers:\n    " + "\n    ".join(bad_list))
         # end if
         return (potential_list, None)
     # end if
@@ -161,52 +161,50 @@ def _is_mult_period(entry):
     >>> _is_mult_period("3 * nmonths")
     ((3, 'nmonths'), None)
     >>> _is_mult_period("2*fortnights")
-    (None, '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')
+    (None, 'period ("fortnights") 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')
     >>> _is_mult_period("7*nhours of day")
-    (None, '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')
+    (None, 'period ("nhours of day") 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')
     >>> _is_mult_period(" ")
-    (None, 'a frequency ([<mult>*]period) is required')
+    (None, 'frequency ([<mult>*]period) is required, found " "')
     >>> _is_mult_period("1*nyear")
     ((1, 'nyear'), None)
     >>> _is_mult_period("-6*nhours")
-    (None, "multiplier '-6' in '-6*nhours' must be a positive integer")
+    (None, 'multiplier ("-6") must be a positive integer')
     >>> _is_mult_period("7h*nhours")
-    (None, "multiplier '7h' in '7h*nhours' must be a positive integer")
+    (None, '"7h" in "7h*nhours" is not a valid integer')
     >>> _is_mult_period("5*nhours*ndays")
-    (None, "multiplier '5*nhours' in '5*nhours*ndays' must be a positive integer")
+    (None, 'frequency must be of the form ([<mult>*]period), found "5*nhours*ndays".  Do you have too many multipliers or periods?')
     """
-    if not entry.strip():
-        errmsg = "a frequency ([<mult>*]period) is required"
-        return (None, errmsg)
+    if not entry or not entry.strip():
+        return (None, f"frequency ([<mult>*]period) is required, found \"{entry}\"")
     # end if
-    _OPTIONAL_MULTIPLIER_TOKEN = r"((?P<multiplier>.*)\*)"
-    _REQUIRED_PERIOD_TOKEN = "(?P<period>.*)"
-    _MULT_PERIOD_REGEX = f"^{_OPTIONAL_MULTIPLIER_TOKEN}?\s*{_REQUIRED_PERIOD_TOKEN}$"
-    match = re.match(_MULT_PERIOD_REGEX, entry.strip())
-    group_dict = match.groupdict()
-    if len(group_dict) == 0 or 'period' not in group_dict:
-        errmsg = f"Bad formatting of frequency, '{entry}' must be in the form of '[<integer>*]<time period>'"
-        return (None, errmsg)
-    # end if
-    # Check multiplier
+    tokens = [x.strip() for x in str(entry.strip()).split('*')]
+    num_tokens = len(tokens)
+
     multiplier = 1
-    if 'multiplier' in group_dict:
-        potential_multiplier = group_dict['multiplier']
-        if potential_multiplier:
-            multiplier, errmsg = _is_integer(potential_multiplier)
-            if errmsg or (multiplier <= 0):
-                errmsg = f"multiplier '{potential_multiplier}' in '{entry}' must be a positive integer"
-                return (None, errmsg)
-            # end if
-        # end if
+    period = ""
+    if num_tokens == 1:
+        period = tokens[0].lower()
+    elif num_tokens == 2:
+        multiplier = tokens[0]
+        period = tokens[1].lower()
+    else:
+        return (None, f"frequency must be of the form ([<mult>*]period), found \"{entry}\".  Do you have too many multipliers or periods?")
     # end if
-    # Check for valid period
-    period = group_dict['period'].strip().lower()
+
     if period not in _TIME_PERIODS:
-       errmsg = f'period must be one of {", ".join(_TIME_PERIODS)}'
-       return (None, errmsg)
+        time_periods = ", ".join(_TIME_PERIODS)
+        return (None, f"period (\"{period}\") must be one of {time_periods}")
     # end if
-    return ((multiplier, period), None)
+    (candidate_multiplier, errmsg) = _is_integer(multiplier)
+    if errmsg:
+        return (None, f"\"{multiplier}\" in \"{entry}\" is not a valid integer")
+    # end if
+    if candidate_multiplier <= 0:
+        return (None, f"multiplier (\"{candidate_multiplier}\") must be a positive integer")
+    # end if
+
+    return ((candidate_multiplier, period), None)
 
 ##############################################################################
 def _is_prec_str(entry):
@@ -308,6 +306,8 @@ def _parse_hist_config_line(line, no_command_ok=False):
     (None, None, "Invalid history config line, 'use_topo_file = .false.'")
     >>> _parse_hist_config_line("use_topo_file = .false.", no_command_ok=True)
     ('use_topo_file', None, None)
+    >>> _parse_hist_config_line(" ")
+    (None, None, None)
     """
     # Find the possible history configuration command for <line>.
     if _blank_config_line(line):
@@ -679,7 +679,7 @@ def remove_fields(self, fields, pobj, logger):
             all_removed = False
             missing_fields = fields_to_delete.difference(fields_deleted)
             ctx = context_string(pobj)
-            missing_fields_string = ", ".join(list(missing_fields))
+            missing_fields_string = ", ".join(missing_fields)
             errmsg = f"Cannot remove field(s), '{missing_fields_string}', not found on hist volume, {self.volume}{ctx}"
             logger.warning(errmsg)
         # end if
@@ -772,6 +772,13 @@ def output_frequency(self):
         return self.__output_freq
 
     def __out_frequency_is_valid(this, ofreq):
+        """
+        Determine if a user-supplied output frequency is valid.
+        Checks:
+         - frequency is tuple (multiplier, period)
+         - multiplier is a positive integer
+         - period is in list of valid time periods
+        """
         return isinstance(ofreq, tuple)                   and \
                (len(ofreq) == 2)                          and \
                (ofreq[0] > 0)                             and \

src/dynamics/utils/hycoef.F90

@@ -288,7 +288,8 @@ subroutine hycoef_init(file, psdry)
    if (dry_coord) then
       call add_vert_coord('ilev', pverp,                                     &
          'hybrid level at interfaces (1000*(A+B))', 'hPa', ailev,            &
-         positive='down')
+         positive='down',                                                    &
+         standard_name='atmosphere_hybrid_sigma_pressure_coordinate_at_interfaces')
       call add_hist_coord('hyai', pverp, &
          'hybrid A coefficient at layer interfaces', '1', hyai, dimname='ilev')
       call add_hist_coord('hybi', pverp, &
@@ -309,7 +310,7 @@ subroutine hycoef_init(file, psdry)
       call add_vert_coord('ilev', pverp,                                     &
          'hybrid level at interfaces (1000*(A+B))', 'hPa', ailev,            &
          positive='down',                                                    &
-         standard_name='atmosphere_hybrid_sigma_pressure_coordinate',        &
+         standard_name='atmosphere_hybrid_sigma_pressure_coordinate_at_interfaces', &
          formula_terms=formula_terms)
    end if
 

src/history/cam_hist_file.F90

@@ -50,8 +50,8 @@ module cam_hist_file
       integer,                       private :: output_freq_mult = UNSET_I
       character(len=8),              private :: output_freq_type = UNSET_C
       integer,                       private :: num_samples = 0
-      real(kind=r8),                 private :: beg_time = UNSET_R8
-      real(kind=r8),                 private :: end_time = UNSET_R8
+      real(r8),                      private :: beg_time = UNSET_R8
+      real(r8),                      private :: end_time = UNSET_R8
       character(len=:), allocatable, private :: filename_spec
       character(len=max_fldlen), allocatable, private :: field_names(:)
       character(len=3), allocatable, private :: accumulate_types(:)
@@ -914,7 +914,7 @@ subroutine config_define_file(this, restart, logname, host, model_doi_url)
       use cam_history_support, only: max_chars
       use time_manager,        only: get_ref_date, timemgr_get_calendar_cf
       use time_manager,        only: get_step_size
-      use string_utils,        only: date2yyyymmdd, sec2hms
+      use string_utils,        only: date2yyyymmdd, sec2hms, stringify
       use cam_control_mod,     only: caseid
       use cam_initfiles,       only: ncdata, bnd_topo
       use cam_abortutils,      only: check_allocate, endrun
@@ -967,6 +967,7 @@ subroutine config_define_file(this, restart, logname, host, model_doi_url)
       ! A structure to hold the horizontal dimension and coordinate info
       type(cam_grid_header_info_t), allocatable :: header_info(:)
       integer,          allocatable    :: mdimids(:)
+      integer,          parameter :: max_netcdf_len = 256
       character(len=*), parameter :: subname = 'config_define_file: '
 
       is_initfile = (this%hfile_type == hfile_type_init_value)
@@ -1218,6 +1219,15 @@ subroutine config_define_file(this, restart, logname, host, model_doi_url)
             mdims = this%field_list(field_index)%dimensions()
             mdimsize = size(mdims)
             fname_tmp = strip_suffix(this%field_list(field_index)%diag_name())
+            ! Ensure that fname_tmp is not longer than the maximum length for a
+            !  netcdf file
+            if (len_trim(fname_tmp) > max_netcdf_len) then
+               ! Endrun if the name is too long
+               write(errmsg, *) 'config_define_file: variable name ', trim(fname_tmp), &
+                  ' too long for NetCDF file (len=', stringify((/len(trim(fname_tmp))/)), ' > ', &
+                  stringify((/max_netcdf_len/)), ')'
+               call endrun(errmsg, file=__FILE__, line=__LINE__)
+            end if
             !
             !  Create variables and atributes for fields written out as columns
             !
@@ -1659,7 +1669,7 @@ subroutine read_namelist_entry(unitn, hfile_config, hist_inst_fields,      &
       character(len=max_fldlen), allocatable, intent(inout) :: hist_max_fields(:)
       character(len=max_fldlen), allocatable, intent(inout) :: hist_var_fields(:)
       ! Local variables (namelist)
-      character(len=vlen) :: hist_volume ! h# ir i, not config number
+      character(len=vlen) :: hist_volume
       character(len=vlen) :: hist_precision
       integer             :: hist_max_frames
       character(len=flen) :: hist_output_frequency

src/utils/cam_abortutils.F90

@@ -66,6 +66,7 @@ subroutine cam_register_open_file(file, file_name)
       ! Local variables
       type(open_file_pointer), pointer :: of_ptr
       type(open_file_pointer), pointer :: of_new
+      integer                          :: ierr
       character(len=*),  parameter     :: subname = 'cam_register_open_file'
 
       nullify(of_new)
@@ -80,7 +81,9 @@ subroutine cam_register_open_file(file, file_name)
       ! If we get here, go ahead and register the file
       if (associated(open_files_pool)) then
          of_new => open_files_pool
-         allocate(of_new%file_desc)
+         allocate(of_new%file_desc, stat=ierr)
+         call check_allocate(ierr, subname, 'of_file%file_desc', file=__FILE__, &
+             line=__LINE__)
          of_new%file_desc = file
          of_new%file_name = file_name
          allocate(open_files_pool%next)

src/utils/cam_filenames.F90

@@ -132,6 +132,19 @@ character(len=cl) function interpret_filename_spec(filename_spec, unit, accum_ty
          else
             call get_curr_date(year, month, day, ncsec)
          end if
+      else
+         if (present(yr_spec)) then
+            year = yr_spec
+         end if
+         if (present(mon_spec)) then
+            month = mon_spec
+         end if
+         if (present(day_spec)) then
+            day = day_spec
+         end if
+         if (present(sec_spec)) then
+            ncsec = sec_spec
+         end if
       end if ! No else, do not use these quantities below.
       !
       ! Go through each character in the filename specifier and interpret

test/run_unit_tests.sh

@@ -66,6 +66,8 @@ run_doctest cime_config/create_readnl_files.py
 run_doctest src/data/generate_registry_data.py
 # ParamGen atm_in namelist writer doctests:
 run_doctest cime_config/atm_in_paramgen.py
+# CAM history config doctests:
+run_doctest cime_config/hist_config.py
 # CAM config unit tests:
 run_unittest test/unit/test_cam_config.py
 # CAM autogen unit tests:

test/unit/test_hist_config.py

@@ -80,7 +80,7 @@ def _test_config(self, config, vol, prec, maxf, outfreq, ftype, write_nstep0, fi
         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.write_nstep0, write_nstep0, msg="Bad write_nstep0 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")
 
@@ -188,7 +188,6 @@ def test_bad_user_nl_cam(self):
             # 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"