Repository: ioos/compliance-checker
Pull request #1014 (2023-04-18 22:52:12)

Implement majority of conformance for CF section 4.4
Adds implementations and tests for the following sections:

Required:
- The reference date/time in time units is not allowed to contain seconds equal to or greater than 60.

Recommendations:
- The use of time coordinates in year 0 and reference date/times in year 0 to indicate climatological time is deprecated.
- Units of year and month and any equivalent units should be used with caution.


Patch:
diff --git a/compliance_checker/cf/cf_1_6.py b/compliance_checker/cf/cf_1_6.py
index d4752bf..5a6b4c4 100644
--- a/compliance_checker/cf/cf_1_6.py
+++ b/compliance_checker/cf/cf_1_6.py
@@ -1821,11 +1821,35 @@ class CF1_6Check(CFNCCheck):
             reasoning = None
             if not correct_units:
                 reasoning = ["%s does not have correct time units" % name]
-            result = Result(
-                BaseCheck.HIGH, correct_units, self.section_titles["4.4"], reasoning
-            )
-            ret_val.append(result)
-
+                result = Result(
+                    BaseCheck.HIGH, correct_units, self.section_titles["4.4"], reasoning
+                )
+                ret_val.append(result)
+                continue
+            # IMPLEMENTATION CONFORMANCE 4.4 RECOMMENDED 1/2
+            if hasattr(variable, "climatology"):
+                year_match = regex.match(r"\w+ since (?P<year>\d{1,4})", variable.units)
+                # year should always exist at this point if it's been parsed as
+                # valid date
+                if int(year_match.group("year")) == 0:
+                    message = (
+                        f"Time coordinate variable {variable.name}'s "
+                        "use of year 0 for climatological time is "
+                        "deprecated"
+                    )
+                    result = Result(
+                        BaseCheck.MEDIUM, False, self.section_titles["4.4"], [message]
+                    )
+                    ret_val.append(result)
+            # IMPLEMENTATION CONFORMANCE 4.4 RECOMMENDED 2/2
+            # catch non-recommended months or years time interval
+            unit = Unit(variable.units)
+            if unit.is_long_time_interval():
+                message = f"Using relative time interval of months or years is not recommended for coordinate variable {variable.name}"
+                result = Result(
+                    BaseCheck.MEDIUM, False, self.section_titles["4.4"], [message]
+                )
+                ret_val.append(result)
         return ret_val
 
     def check_calendar(self, ds):
@@ -1870,7 +1894,7 @@ class CF1_6Check(CFNCCheck):
         :rtype: list
         :return: List of results
         """
-        valid_calendars = {
+        standard_calendars = {
             "gregorian",
             "standard",
             "proleptic_gregorian",
@@ -1896,10 +1920,30 @@ class CF1_6Check(CFNCCheck):
             # should be made for time coordinate variables anyways, so errors
             # should be caught where implemented there
             crossover_date = cftime.DatetimeGregorian(1582, 10, 15)
-            times = cftime.num2date(time_var[:].compressed(), time_var.units)
+            # has_year_zero set to true in order to just check crossover,
+            # actual year less than or equal to zero check handled elsewhere
+            # when standard/Gregorian, or Julian calendars used.
 
-            no_cross_1582 = ~np.any(times < crossover_date)
-            if no_cross_1582:
+            # WARNING: might fail here if months_since are used and suppress
+            #          usual warning
+            try:
+                times = cftime.num2date(
+                    time_var[:].compressed(), time_var.units, has_year_zero=True
+                )
+            except ValueError:
+                return Result(
+                    BaseCheck.LOW,
+                    False,
+                    self.section_titles["4.4"],
+                    [
+                        "Miscellaneous failure when attempting to calculate crossover, possible malformed date"
+                    ],
+                )
+
+            crossover_1582 = np.any(times < crossover_date) and np.any(
+                times >= crossover_date
+            )
+            if not crossover_1582:
                 reasoning = (
                     f"Variable {time_var.name} has standard or Gregorian "
                     "calendar and does not cross 1582-10-15T00:00Z"
@@ -1912,15 +1956,22 @@ class CF1_6Check(CFNCCheck):
                 )
 
             return Result(
-                BaseCheck.LOW, no_cross_1582, self.section_titles["4.4"], [reasoning]
+                BaseCheck.LOW,
+                not crossover_1582,
+                self.section_titles["4.4"],
+                [reasoning],
             )
 
         # if has a calendar, check that it is within the valid values
         # otherwise no calendar is valid
 
-        for time_var in ds.get_variables_by_attributes(
-            calendar=lambda c: c is not None
-        ):
+        # this will only fetch variables with time units defined
+        for time_var_name in cfutil.get_time_variables(ds):
+            if time_var_name not in {var.name for var in util.find_coord_vars(ds)}:
+                continue
+            time_var = ds.variables[time_var_name]
+            if not hasattr(time_var, "calendar"):
+                continue
             if time_var.calendar.lower() == "gregorian":
                 reasoning = (
                     f"For time variable {time_var.name}, when using "
@@ -1929,7 +1980,7 @@ class CF1_6Check(CFNCCheck):
                     "the calendar attribute"
                 )
                 result = Result(
-                    BaseCheck.LOW, False, self.section_titles["4.4"], [reasoning]
+                    BaseCheck.LOW, False, self.section_titles["4.4.1"], [reasoning]
                 )
                 ret_val.append(result)
                 # check here and in the below case that time does not cross
@@ -1939,12 +1990,12 @@ class CF1_6Check(CFNCCheck):
                 ret_val.append(check_standard_calendar_no_cross(time_var))
             # if a nonstandard calendar, then leap_years and leap_months must
             # must be present
-            if time_var.calendar.lower() not in valid_calendars:
+            if time_var.calendar.lower() not in standard_calendars:
                 result = self._check_leap_time(time_var)
             # passes if the calendar is valid, otherwise notify of invalid
             # calendar
             else:
-                result = Result(BaseCheck.LOW, True, self.section_titles["4.4"], None)
+                result = Result(BaseCheck.LOW, True, self.section_titles["4.4.1"])
             ret_val.append(result)
 
         return ret_val
diff --git a/compliance_checker/cf/cf_1_9.py b/compliance_checker/cf/cf_1_9.py
index c499448..5419285 100644
--- a/compliance_checker/cf/cf_1_9.py
+++ b/compliance_checker/cf/cf_1_9.py
@@ -1,7 +1,10 @@
+import cftime
+import regex
 from netCDF4 import Dataset
 
 from compliance_checker import cfutil
-from compliance_checker.base import BaseCheck, TestCtx
+from compliance_checker.base import BaseCheck, Result, TestCtx
+from compliance_checker.cf import util
 from compliance_checker.cf.cf_1_8 import CF1_8Check
 from compliance_checker.cf.util import VariableReferenceError, reference_attr_variables
 
@@ -14,29 +17,82 @@ class CF1_9Check(CF1_8Check):
         super(CF1_9Check, self).__init__(options)
         self.section_titles.update({"5.8": "§5.8 Domain Variables"})
 
-    def check_calendar(self, ds):
-        # IMPLEMENTATION CONFORMANCE 4.4.1 RECOMMENDED CF 1.9
-        super(CF1_9Check, self).check_calendar.__doc__
-        prev_return = super(CF1_9Check, self).check_calendar(ds)
-        time_var_candidate_name = cfutil.get_time_variable(ds)
-        time_var_name = (
-            time_var_candidate_name
-            if time_var_candidate_name in self._find_coord_vars(ds)
-            else None
-        )
-        # most datasets should have a time coordinate variable
-        test_ctx = self.get_test_ctx(
-            BaseCheck.HIGH, self.section_titles["4.4"], time_var_name
-        )
-        if time_var_name is None:
-            return prev_return
+    def check_time_coordinate_variable_has_calendar(self, ds):
+        """
+        Ensure that time coordinate variables have a calendar attribute
+        """
+        ret_val = []
+        for name in cfutil.get_time_variables(ds):
+            # DRY: get rid of time coordinate variable boilerplate
+            if name not in {var.name for var in util.find_coord_vars(ds)}:
+                continue
+            time_var = ds.variables[name]
+            if not hasattr(time_var, "calendar") or not isinstance(
+                time_var.calendar, str
+            ):
+                result = Result(
+                    BaseCheck.MEDIUM,
+                    True,
+                    self.section_titles["4.4.1"],
+                    [
+                        f'Time coordinate variable "{name}" should have a '
+                        'string valued attribute "calendar"'
+                    ],
+                )
+                ret_val.append(result)
+                continue
+            if time_var.calendar.lower() in {"gregorian", "julian", "standard"}:
+                try:
+                    reference_year = cftime.num2date(
+                        0, time_var.units, time_var.calendar, has_year_zero=True
+                    ).year
+                # will fail on months, certain other time specifications
+                except ValueError:
+                    continue
+                if reference_year == 0:
+                    reasoning = (
+                        f'For time variable "{time_var.name}", when using '
+                        "the Gregorian or Julian calendars, the use of year "
+                        "zero is not recommended. Furthermore, the use of year "
+                        "zero to signify a climatological variable as in COARDS "
+                        "is deprecated in CF."
+                    )
+                    result = Result(
+                        BaseCheck.MEDIUM,
+                        False,
+                        self.section_titles["4.4.1"],
+                        [reasoning],
+                    )
+
+                    ret_val.append(result)
+        return ret_val
 
-        test_ctx.assert_true(
-            hasattr(ds.variables[time_var_name], "calendar"),
-            f'Time coordinate variable "{time_var_name}" '
-            "should have a calendar attribute",
+    def check_time_coordinate(self, ds):
+        super(CF1_9Check, self).check_calendar.__doc__
+        prev_return = super(CF1_9Check, self).check_time_coordinate(ds)
+        seconds_regex = regex.compile(
+            r"\w+ since \d{1,4}-\d{1,2}-\d{1,2}[ T]"
+            r"\d{1,2}:\d{1,2}:(?P<seconds>\d{1,2})"
         )
-        prev_return.append(test_ctx.to_result())
+        for name in cfutil.get_time_variables(ds):
+            # DRY: get rid of time coordinate variable boilerplate
+            if name not in {var.name for var in util.find_coord_vars(ds)}:
+                continue
+            time_var = ds.variables[name]
+            test_ctx = self.get_test_ctx(
+                BaseCheck.HIGH, self.section_titles["4.4"], name
+            )
+            try:
+                match = regex.match(seconds_regex, time_var.units)
+            except AttributeError:
+                # not much can be done if there are no units
+                continue
+            test_ctx.assert_true(
+                match.group("seconds") is None or int(match.group("seconds")) < 60,
+                f'Time coordinate variable "{name}" must have '
+                "units with seconds less than 60",
+            )
+            prev_return.append(test_ctx.to_result())
         return prev_return
 
     def check_domain_variables(self, ds: Dataset):
diff --git a/compliance_checker/cf/cf_base.py b/compliance_checker/cf/cf_base.py
index 3fe3f1d..a747c91 100644
--- a/compliance_checker/cf/cf_base.py
+++ b/compliance_checker/cf/cf_base.py
@@ -58,6 +58,7 @@ class CFBaseCheck(BaseCheck):
             "4.2": "§4.2 Longitude Coordinate",
             "4.3": "§4.3 Vertical Coordinate",
             "4.4": "§4.4 Time Coordinate",
+            "4.4.1": "§4.4.1 Calendar",
             "4.5": "§4.5 Discrete Axis",
             "5": "§5 Coordinate Systems",
             "5.1": "§5.1 Independent Latitude, Longitude, Vertical, and Time Axes",


Test patch:
diff --git a/compliance_checker/tests/helpers.py b/compliance_checker/tests/helpers.py
index 9852ad5..a15e954 100644
--- a/compliance_checker/tests/helpers.py
+++ b/compliance_checker/tests/helpers.py
@@ -31,7 +31,7 @@ class MockTimeSeries(MockNetCDF):
         super(MockTimeSeries, self).__init__(filename)
         self.createDimension("time", 500)
         for name, std_name, units, axis in (
-            ("time", "time", "seconds since 1970-01-01", "T"),
+            ("time", "time", "seconds since 1970-01-01 00:00:00", "T"),
             ("lon", "longitude", "degrees_east", "X"),
             ("lat", "latitude", "degrees_north", "Y"),
             ("depth", "depth", "m", "Z"),
@@ -44,14 +44,7 @@ class MockTimeSeries(MockNetCDF):
             var.axis = axis
 
         # give some applicable units
-        self.variables["time"].units = "seconds since 2019-04-11T00:00:00"
-        self.variables["time"].axis = "T"
-        self.variables["lat"].units = "degree_north"
-        self.variables["lat"].axis = "Y"
-        self.variables["lon"].units = "degree_east"
-        self.variables["lon"].axis = "X"
-        self.variables["depth"].units = "meters"
-        self.variables["depth"].axis = "Z"
+        self.variables["time"].calendar = "standard"
         self.variables["depth"].positive = "down"
 
 
diff --git a/compliance_checker/tests/test_cf.py b/compliance_checker/tests/test_cf.py
index 388053b..a29b7c4 100644
--- a/compliance_checker/tests/test_cf.py
+++ b/compliance_checker/tests/test_cf.py
@@ -36,6 +36,7 @@ from compliance_checker.cf.util import (
 from compliance_checker.suite import CheckSuite
 from compliance_checker.tests import BaseTestCase
 from compliance_checker.tests.helpers import (
+    MockNetCDF,
     MockRaggedArrayRepr,
     MockTimeSeries,
     MockVariable,
@@ -1233,6 +1234,7 @@ class TestCF1_6(BaseTestCase):
         for r in results:
             self.assertTrue(r.value)
 
+        # TEST CONFORMANCE 4.4 REQUIRED 1/2
         dataset = self.load_dataset(STATIC_FILES["bad"])
         results = self.cf.check_time_coordinate(dataset)
 
@@ -1240,6 +1242,23 @@ class TestCF1_6(BaseTestCase):
 
         assert "time does not have correct time units" in messages
         assert (scored, out_of) == (1, 2)
+        # TEST CONFORMANCE 4.4 REQUIRED 2/2, RECOMMENDED 1, 2/2
+        dataset = MockTimeSeries()
+        # NB: >= 60 seconds is nonstandard, but isn't actually a CF requirement
+        # until CF 1.9 onwards
+        dataset.variables["time"].units = "months since 0-1-1 23:00:60"
+        dataset.variables[
+            "time"
+        ].climatology = (
+            "nonexistent_variable_reference_only_used_to_test_year_zero_failure"
+        )
+        results = self.cf.check_time_coordinate(dataset)
+        scored, out_of, messages = get_results(results)
+        assert scored < out_of
+        assert (
+            "Using relative time interval of months or years is not recommended for coordinate variable time"
+            in messages
+        )
 
     def test_check_calendar(self):
         """Load a dataset with an invalid calendar attribute (non-comp/bad.nc).
@@ -1261,7 +1280,12 @@ class TestCF1_6(BaseTestCase):
         assert bad_month_msg in messages
 
         dataset = MockTimeSeries()
-        dataset.variables["time"]
+        # no calendar should not raise an issue on time coordinate variables
+        del dataset.variables["time"].calendar
+        results = self.cf.check_calendar(dataset)
+        scored, out_of, messages = get_results(results)
+        assert not messages
+
         # test case insensivity
         valid_calendars = (
             "GREGORIAN",
@@ -1276,6 +1300,10 @@ class TestCF1_6(BaseTestCase):
             "NONE",
         )
         for calendar_uppercase in valid_calendars:
+            # need to make a new MockTimeSeries when attribute deleted for
+            # calendar attributes to work properly
+            dataset = MockTimeSeries()
+            dataset.calendar = calendar_uppercase
             results = self.cf.check_calendar(dataset)
             scored, out_of, messages = get_results(results)
             assert scored == out_of
@@ -2997,30 +3025,69 @@ class TestCF1_9(BaseTestCase):
     def setUp(self):
         self.cf = CF1_9Check()
 
+    def test_time_variable_over_sixty_seconds(self):
+        dataset = MockTimeSeries()
+        # TEST CF CONFORMANCE 4.4 REQUIRED
+        dataset.variables["time"].units = "months since 0-1-1 23:00:60"
+        results = self.cf.check_time_coordinate(dataset)
+        scored, out_of, messages = get_results(results)
+        assert (
+            'Time coordinate variable "time" must have units with seconds less than 60'
+            in messages
+        )
+
     def test_time_variable_has_calendar(self):
+        self.cf = CF1_9Check()
         # TEST CONFORMANCE 4.4.1 RECOMMENDED CF 1.9
         dataset = MockTimeSeries()
-        results = self.cf.check_calendar(dataset)
+        del dataset.variables["time"].calendar
+        results = self.cf.check_time_coordinate_variable_has_calendar(dataset)
+        scored, out_of, messages = get_results(results)
         assert (
-            results[0].msgs[0] == 'Time coordinate variable "time" should have a '
-            "calendar attribute"
+            'Time coordinate variable "time" should have a string valued attribute "calendar"'
+            in messages
         )
         # FIXME: NetCDF files shouldn't normally be modified so we can usually
         # depend on cached results. Here we need to recreate the checker
         # instance in order to not have previous results included pass condition
-        self.cf = CF1_9Check()
         dataset.variables["time"].calendar = "standard"
         results = self.cf.check_calendar(dataset)
         # no time coordinate present, i.e. there is no time variable name with
         # the same name as the time dimension name.
         self.cf = CF1_9Check()
-        dataset = MockTimeSeries()
-        dataset.variables["time2"] = dataset.variables["time"]
-        del dataset.variables["time"]
+        # need to manually construct the netCDF object here --
+        # get_variables_by_attributes appears to be interfering here
+        dataset = MockNetCDF()
+        dataset.createDimension("time", 500)
+        dataset.createVariable("time2", "f8", ("time",))
+        dataset.variables["time2"].standard_name = "time"
+        dataset.variables["time2"].units = "seconds since 1970-01-01 00:00:00"
+        dataset.variables["time2"].axis = "T"
         results = self.cf.check_calendar(dataset)
         # results array should be empty as no time coordinate variable detected
         assert not results
 
+        # TEST CONFORMANCE 4.4.1
+        dataset = MockTimeSeries()
+        dataset.variables["time"].units = "months since 0-1-1 23:00:60"
+        results = self.cf.check_calendar(dataset)
+        scored, out_of, messages = get_results(results)
+
+        # test greater than or equal to one zero year for Julian and Gregorian
+        # calendars
+        dataset = MockTimeSeries()
+        dataset.variables["time"].units = "seconds since 0-01-01 00:00:00"
+        for calendar_name in ("standard", "julian", "gregorian"):
+            dataset.variables["time"].calendar = calendar_name
+            results = self.cf.check_time_coordinate_variable_has_calendar(dataset)
+            scored, out_of, messages = get_results(results)
+            assert (
+                'For time variable "time", when using the Gregorian or Julian '
+                "calendars, the use of year zero is not recommended. "
+                "Furthermore, the use of year zero to signify a climatological "
+                "variable as in COARDS is deprecated in CF." in messages
+            )
+
     def test_domain(self):
         dataset = MockTimeSeries()
         domain_var = dataset.createVariable("domain", "c", ())
diff --git a/compliance_checker/tests/test_cf_integration.py b/compliance_checker/tests/test_cf_integration.py
index 3b102a4..19fccba 100644
--- a/compliance_checker/tests/test_cf_integration.py
+++ b/compliance_checker/tests/test_cf_integration.py
@@ -22,7 +22,7 @@ dataset_stem__expected_messages = [
             "attribute lat:_CoordianteAxisType should begin with a letter and be composed of letters, digits, and underscores",
             "attribute lon:_CoordianteAxisType should begin with a letter and be composed of letters, digits, and underscores",
             "§2.6.2 global attribute history should exist and be a non-empty string",
-            "standard_name temperature is not defined in Standard Name Table v{}".format(
+            "standard_name temperature is not defined in Standard Name Table v{}. Possible close match(es): ['air_temperature', 'soil_temperature', 'snow_temperature']".format(
                 std_names._version
             ),
             "temperature's auxiliary coordinate specified by the coordinates attribute, precise_lat, is not a variable in this dataset",
@@ -46,28 +46,28 @@ dataset_stem__expected_messages = [
             "Attribute 'valid_range' (type: <class 'numpy.int16'>) and parent variable 'wind_direction_qc' (type: <class 'numpy.int8'>) must have equivalent datatypes",
             "Attribute 'valid_range' (type: <class 'numpy.int16'>) and parent variable 'visibility_qc' (type: <class 'numpy.int8'>) must have equivalent datatypes",
             '§2.6.1 Conventions global attribute does not contain "CF-1.8"',
-            "standard_name visibility is not defined in Standard Name Table v{}".format(
+            "standard_name visibility is not defined in Standard Name Table v{}. Possible close match(es): ['visibility_in_air']".format(
                 std_names._version
             ),
             'Standard name modifier "data_quality" for variable visibility_qc is not a valid modifier according to CF Appendix C',
-            "standard_name wind_direction is not defined in Standard Name Table v{}".format(
+            "standard_name wind_direction is not defined in Standard Name Table v{}. Possible close match(es): ['wind_to_direction', 'wind_from_direction', 'wind_gust_from_direction']".format(
                 std_names._version
             ),
             'Standard name modifier "data_quality" for variable wind_direction_qc is not a valid modifier according to CF Appendix C',
-            "standard_name wind_gust is not defined in Standard Name Table v{}".format(
+            "standard_name wind_gust is not defined in Standard Name Table v{}. Possible close match(es): ['y_wind_gust', 'x_wind_gust', 'wind_speed_of_gust']".format(
                 std_names._version
             ),
             'Standard name modifier "data_quality" for variable wind_gust_qc is not a valid modifier according to CF Appendix C',
             'Standard name modifier "data_quality" for variable air_temperature_qc is not a valid modifier according to CF Appendix C',
-            "standard_name use_wind is not defined in Standard Name Table v{}".format(
+            "standard_name use_wind is not defined in Standard Name Table v{}. Possible close match(es): ['y_wind', 'x_wind']".format(
                 std_names._version
             ),
-            "standard_name barometric_pressure is not defined in Standard Name Table v{}".format(
+            "standard_name barometric_pressure is not defined in Standard Name Table v{}. Possible close match(es): ['air_pressure', 'reference_pressure', 'barometric_altitude']".format(
                 std_names._version
             ),
             'Standard name modifier "data_quality" for variable barometric_pressure_qc is not a valid modifier according to CF Appendix C',
             'Standard name modifier "data_quality" for variable wind_speed_qc is not a valid modifier according to CF Appendix C',
-            "standard_name barometric_pressure is not defined in Standard Name Table v{}".format(
+            "standard_name barometric_pressure is not defined in Standard Name Table v{}. Possible close match(es): ['air_pressure', 'reference_pressure', 'barometric_altitude']".format(
                 std_names._version
             ),
             "CF recommends latitude variable 'lat' to use units degrees_north",
@@ -152,10 +152,10 @@ dataset_stem__expected_messages = [
         [
             # TODO: referenced/relative time is treated like time units
             'Units "hours since 2016-01-01T12:00:00Z" for variable time_offset must be convertible to canonical units "s"',
-            "standard_name cloud_cover is not defined in Standard Name Table v{}".format(
+            "standard_name cloud_cover is not defined in Standard Name Table v{}. Possible close match(es): ['land_cover', 'land_cover_lccs', 'cloud_albedo']".format(
                 std_names._version
             ),
-            "standard_name dew_point is not defined in Standard Name Table v{}".format(
+            "standard_name dew_point is not defined in Standard Name Table v{}. Possible close match(es): ['dew_point_depression', 'dew_point_temperature']".format(
                 std_names._version
             ),
             (