Repository: ioos/compliance-checker
Pull request #909 (2022-03-08 20:43:52)

Standard name units and modifiers
Fix handling of standard name modifiers, including deprecation warnings in CF >= 1.7.


Patch:
diff --git a/compliance_checker/cf/appendix_c.py b/compliance_checker/cf/appendix_c.py
new file mode 100644
index 0000000..356d5d8
--- /dev/null
+++ b/compliance_checker/cf/appendix_c.py
@@ -0,0 +1,10 @@
+"""Appendix C - Standard Name modifiers"""
+
+# Dict of standard name modifiers with values for units.  "u" inidicates to use
+# the same units as the standard name canonical units, "1" is unitless for
+# observation counts, and None is used for status_flag, which expects units
+# not to be present
+valid_modifiers = {"detection_minimum": "u",
+                   "number_of_observations": "1",
+                   "standard_error": "u",
+                   "status_flag": None}
diff --git a/compliance_checker/cf/cf_1_6.py b/compliance_checker/cf/cf_1_6.py
index 6f3898a..610cff0 100644
--- a/compliance_checker/cf/cf_1_6.py
+++ b/compliance_checker/cf/cf_1_6.py
@@ -14,6 +14,7 @@ from cf_units import Unit
 from compliance_checker import cfutil
 from compliance_checker.base import BaseCheck, BaseNCCheck, Result, TestCtx
 from compliance_checker.cf import util
+from compliance_checker.cf.appendix_c import valid_modifiers
 from compliance_checker.cf.appendix_d import (dimless_vertical_coordinates_1_6,
                                               no_missing_terms)
 from compliance_checker.cf.appendix_e import cell_methods16
@@ -145,9 +146,9 @@ class CF1_6Check(CFNCCheck):
             val = (
                 att.dtype == variable.dtype
             ) or (  # will short-circuit or if first condition is true
-                isinstance(att, (np.float32, np.float64, float))
-                and variable.dtype in (np.byte, np.short, np.int16, np.int,
-                                       np.int32, int))
+                isinstance(att.dtype, (np.float, np.double, float))
+                and isinstance(variable.dtype, (np.byte, np.short, np.int, int))
+            )
         if not val:
             msgs.append(error_msg)
 
@@ -170,18 +171,6 @@ class CF1_6Check(CFNCCheck):
             scale_factor=lambda x: x is not None
         )
 
-        _attr_vars_tup = []
-        both = set(add_offset_vars).intersection(scale_factor_vars)
-        both_msgs = []
-        for both_var in sorted(both, key=lambda var: var.name):
-            if (both_var.scale_factor.dtype !=
-                both_var.add_offset.dtype):
-                both_msgs.append("When both scale_factor and add_offset "
-                                 f"are supplied for variable {both_var.name}, "
-                                 "they must have the same type")
-        results.append(Result(BaseCheck.MEDIUM, not bool(both_msgs),
-                       self.section_titles["8.1"], both_msgs))
-
         for _att_vars_tup in (
             ("add_offset", add_offset_vars),
             ("scale_factor", scale_factor_vars),
@@ -561,6 +550,7 @@ class CF1_6Check(CFNCCheck):
         coordinate_variables = self._find_coord_vars(ds)
         auxiliary_coordinates = self._find_aux_coord_vars(ds)
         geophysical_variables = self._find_geophysical_vars(ds)
+        modifier_variables = cfutil._find_standard_name_modifier_variables(ds)
         forecast_variables = cfutil.get_forecast_metadata_variables(ds)
 
         unit_required_variables = set(
@@ -568,6 +558,7 @@ class CF1_6Check(CFNCCheck):
             + auxiliary_coordinates
             + geophysical_variables
             + forecast_variables
+            + modifier_variables # standard names with modifiers require proper units, *except* for flags, where they should not be present
         )
 
         for name in unit_required_variables:
@@ -598,11 +589,12 @@ class CF1_6Check(CFNCCheck):
             valid_units = self._check_valid_cf_units(ds, name)
             ret_val.append(valid_units)
 
-            units_attr_is_string = TestCtx(BaseCheck.MEDIUM, self.section_titles["3.1"])
+            units_attr_is_string = TestCtx(BaseCheck.MEDIUM,
+                                           self.section_titles["3.1"])
 
             # side effects, but better than teasing out the individual result
             if units_attr_is_string.assert_true(
-                isinstance(units, str),
+                isinstance(units, str) or units is None,
                 "units ({}) attribute of '{}' must be a string compatible with UDUNITS".format(
                     units, variable.name
                 ),
@@ -632,6 +624,7 @@ class CF1_6Check(CFNCCheck):
         deprecated = ["level", "layer", "sigma_level"]
         variable = ds.variables[variable_name]
 
+        valid_units = TestCtx(BaseCheck.HIGH, self.section_titles["3.1"])
         units = getattr(variable, "units", None)
         standard_name_full = getattr(variable, "standard_name", None)
         standard_name, standard_name_modifier = self._split_standard_name(
@@ -640,6 +633,42 @@ class CF1_6Check(CFNCCheck):
         std_name_units_dimensionless = cfutil.is_dimensionless_standard_name(
             self._std_names._root, standard_name
         )
+
+        # 3) units are not deprecated
+        valid_units.assert_true(
+            units not in deprecated,
+            'units for {}, "{}" are deprecated by CF 1.6'.format(variable_name, units),
+        )
+        # 4/5) Modifiers, if present, have the approriate units, or none for
+        #    status_flag
+        if standard_name_modifier is not None:
+            if standard_name_modifier not in valid_modifiers:
+                valid_units.out_of += 1
+                message = (f"Standard name modifier {standard_name_modifier} is not one of "
+                           f"{valid_modifiers.keys()}")
+                valid_units.msgs.append(message)
+            else:
+                unit_type = valid_modifiers[standard_name_modifier]
+        # no modifiers, just check against standard name canonical_units
+        else:
+            unit_type = "u"
+
+        if unit_type == "u":
+            try:
+                reference = (self._std_names[standard_name].
+                             canonical_units)
+            # if standard name isn't found, there won't be an associated units
+            # but a standard name error will be raised elsewhere
+            except KeyError:
+                return valid_units.to_result()
+        elif unit_type == "1":
+                reference = "1"
+        elif unit_type is None:
+            valid_units.assert_true(units is None,
+                                    f"units attribute for variable {variable_name} must be unset "
+                                    "when status_flag standard name modifier is set")
+            return valid_units.to_result()
+
         # Is this even in the database? also, if there is no standard_name,
         # there's no way to know if it is dimensionless.
         should_be_dimensionless = (
@@ -650,7 +679,6 @@ class CF1_6Check(CFNCCheck):
         )
 
         # 1) Units must exist
-        valid_units = TestCtx(BaseCheck.HIGH, self.section_titles["3.1"])
         valid_units.assert_true(
             should_be_dimensionless or units is not None,
             "units attribute is required for {} when variable is not a dimensionless quantity".format(
@@ -661,17 +689,28 @@ class CF1_6Check(CFNCCheck):
         # Don't bother checking the rest
         if units is None and not should_be_dimensionless:
             return valid_units.to_result()
+
         # 2) units attribute must be a string
         valid_units.assert_true(
             should_be_dimensionless or isinstance(units, str),
             "units attribute for {} needs to be a string".format(variable_name),
         )
 
-        # 3) units are not deprecated
-        valid_units.assert_true(
-            units not in deprecated,
-            'units for {}, "{}" are deprecated by CF 1.6'.format(variable_name, units),
-        )
+        try:
+            units_conv = Unit(units)
+        except ValueError:
+            valid_units.messages.append(f'Unit string "{units}" is not recognized by UDUnits')
+            valid_units.out_of += 1
+            return valid_units
+        else:
+            valid_units.score += 1
+            valid_units.out_of += 1
+
+        # time has special unit handling rules
+        if standard_name != "time":
+            valid_units.assert_true(units_conv.is_convertible(Unit(reference)),
+                                    f'Units "{units}" must be convertible to canonical units '
+                                    f'"{reference}"')
 
         return valid_units.to_result()
 
@@ -701,7 +740,7 @@ class CF1_6Check(CFNCCheck):
         valid_udunits = TestCtx(BaseCheck.HIGH, self.section_titles["3.1"])
         are_udunits = units is not None and util.units_known(units)
         valid_udunits.assert_true(
-            should_be_dimensionless or are_udunits,
+            should_be_dimensionless or are_udunits or units is None,
             'units for {}, "{}" are not recognized by UDUNITS'.format(
                 variable_name, units
             ),
@@ -740,15 +779,27 @@ class CF1_6Check(CFNCCheck):
         # Other standard_name modifiers have the same units as the
         # unmodified standard name or are not checked for units.
 
+        # number_of_observations is a special case which always must be units
+        # of "1"
         if standard_name_modifier == "number_of_observations":
-            canonical_units = "1"
+            valid_standard_units.out_of += 1
+            if units != "1":
+                err_msg = (f"When variable {variable_name} has a "
+                           "standard name modifier of number_of_observations, "
+                           "the specified units must be 1")
+                valid_standard_units.messages.append(err_msg)
+            else:
+                valid_standard_units.score += 1
+            # number_of_observations should short circuit and not continue
+            # on to further units checks
+            return valid_standard_units.to_result()
 
         # This section represents the different cases where simple udunits
         # comparison isn't comprehensive enough to determine if the units are
         # appropriate under CF
 
         # UDUnits accepts "s" as a unit of time but it should be <unit> since <epoch>
-        if standard_name == "time":
+        elif standard_name == "time":
             valid_standard_units.assert_true(
                 util.units_convertible(units, "seconds since 1970-01-01"),
                 "time must be in a valid units format <unit> since <epoch> "
@@ -775,16 +826,6 @@ class CF1_6Check(CFNCCheck):
                 "or degrees if defining a transformed grid. Currently "
                 "{}".format(variable_name, units),
             )
-        # Standard Name table agrees the unit should be dimensionless
-        elif std_name_units_dimensionless:
-            valid_standard_units.assert_true(True, "")
-
-        elif canonical_units is not None:
-            valid_standard_units.assert_true(
-                util.units_convertible(canonical_units, units),
-                "units for variable {} must be convertible to {} "
-                "currently they are {}".format(variable_name, canonical_units, units),
-            )
 
         return valid_standard_units.to_result()
 
@@ -875,16 +916,10 @@ class CF1_6Check(CFNCCheck):
                 # 2) optional - if modifiers, should be in table
                 if standard_name_modifier is not None:
                     valid_modifier = TestCtx(BaseCheck.HIGH, self.section_titles["3.3"])
-                    allowed = [
-                        "detection_minimum",
-                        "number_of_observations",
-                        "standard_error",
-                        "status_flag",
-                    ]
                     valid_modifier.assert_true(
-                        standard_name_modifier in allowed,
-                        "standard_name modifier {} for variable {} is not a valid modifier "
-                        "according to appendix C".format(standard_name_modifier, name),
+                        standard_name_modifier in valid_modifiers,
+                        'Standard name modifier "{}" for variable {} is not a valid modifier '
+                        "according to CF Appendix C".format(standard_name_modifier, name),
                     )
 
                     ret_val.append(valid_modifier.to_result())
diff --git a/compliance_checker/cf/cf_1_7.py b/compliance_checker/cf/cf_1_7.py
index 486ff6d..10b4e71 100644
--- a/compliance_checker/cf/cf_1_7.py
+++ b/compliance_checker/cf/cf_1_7.py
@@ -783,6 +783,16 @@ class CF1_7Check(CF1_6Check):
 
         return prev_return
 
+    def check_standard_name_deprecated_modifiers(self, ds):
+        """
+        Not a standard check in that it won't raise pass/fail values,
+        but instead warns upon finding deprecated CF standard name modifiers.
+        :param netCDF4.Dataset ds: netCDF dataset
+        """
+        deprecated_var_names = cfutil._find_standard_name_modifier_variables(ds, True)
+        if deprecated_var_names:
+            warn(f"Deprecated standard_name modifiers found on variables {deprecated_var_names}")
+
     def _process_v_datum_str(self, v_datum_str, conn):
         vdatum_query = """SELECT 1 FROM alias_name WHERE
                             table_name = 'vertical_datum' AND
diff --git a/compliance_checker/cf/util.py b/compliance_checker/cf/util.py
index 4e97a38..59a257b 100644
--- a/compliance_checker/cf/util.py
+++ b/compliance_checker/cf/util.py
@@ -575,6 +575,36 @@ def is_vertical_coordinate(var_name, var):
         satisfied |= getattr(var, "positive", "").lower() in ("up", "down")
     return satisfied
 
+def compare_unit_types(specified, reference):
+    """
+    Compares two unit strings via UDUnits
+
+    :param str specified: The specified unit
+    :param str reference: The reference unit which to compare agains
+
+    """
+    msgs = []
+    err_flag = False
+    try:
+        specified_unit = Unit(specified)
+    except ValueError:
+        msgs.append(f"Specified conversion unit f{specified} may not be valid UDUnits")
+        err_flag = True
+
+    try:
+        reference_unit = Unit(reference)
+    except ValueError:
+        msgs.append(f"Specified conversion unit f{reference} may not be valid UDUnits")
+        err_flag = True
+
+    if err_flag:
+        return msgs
+
+    unit_convertible = specified_unit.is_convertible(reference_unit)
+    fail_msg = [f'Units "{specified}" are not convertible to "{reference}"']
+    return msgs if unit_convertible else fail_msg
+
+
 
 def string_from_var_type(variable):
     if isinstance(variable, str):
diff --git a/compliance_checker/cfutil.py b/compliance_checker/cfutil.py
index 414e473..2013b5d 100644
--- a/compliance_checker/cfutil.py
+++ b/compliance_checker/cfutil.py
@@ -100,7 +100,7 @@ def attr_membership(attr_val, value_set, attr_type=str, modifier_fn=lambda x: x)
 
 
 @lru_cache(128)
-def is_dimensionless_standard_name(xml_tree, standard_name):
+def is_dimensionless_standard_name(standard_name_table, standard_name):
     """
     Returns True if the units for the associated standard name are
     dimensionless.  Dimensionless standard names include those that have no
@@ -110,15 +110,11 @@ def is_dimensionless_standard_name(xml_tree, standard_name):
     # standard_name must be string, so if it is not, it is *wrong* by default
     if not isinstance(standard_name, str):
         return False
-    found_standard_name = xml_tree.find(".//entry[@id='{}']".format(standard_name))
+    found_standard_name = standard_name_table.find(".//entry[@id='{}']".format(standard_name))
     if found_standard_name is not None:
-        canonical_units = found_standard_name.find("canonical_units")
-        # so far, standard name XML table includes
-        # 1 and 1e-3 for constant units, but expanding to valid udunits
-        # prefixes to be on the safe side
-        # taken from CF Table 3.1 of valid UDUnits prefixes
-        dimless_units = r"1(?:e-?(?:1|2|3|6|9|12|15|18|21|24))?$"
-        return canonical_units is None or re.match(dimless_units, canonical_units.text)
+        canonical_units = (Unit(found_standard_name.find("canonical_units").
+                                text))
+        return canonical_units.is_dimensionless()
     # if the standard name is not found, assume we need units for the time being
     else:
         return False
@@ -731,6 +727,21 @@ def get_climatology_variable(ds):
     return None
 
 
+@lru_cache(128)
+def _find_standard_name_modifier_variables(ds, return_deprecated=False):
+    def match_modifier_variables(standard_name_str):
+        if standard_name_str is None:
+            return False
+        if not return_deprecated:
+            matches = re.search(r"^\w+ +\w+", standard_name_str)
+        else:
+            print(standard_name_str)
+            matches = re.search(r"^\w+ +(?:status_flag|number_of_observations)$", standard_name_str)
+        return bool(matches)
+    return [var.name for var in
+            ds.get_variables_by_attributes(standard_name=
+                                           match_modifier_variables)]
+
 def get_flag_variables(ds):
     """
     Returns a list of variables that are defined as flag variables


Test patch:
diff --git a/compliance_checker/tests/test_cf.py b/compliance_checker/tests/test_cf.py
index f864c77..cb32d4c 100644
--- a/compliance_checker/tests/test_cf.py
+++ b/compliance_checker/tests/test_cf.py
@@ -458,6 +458,21 @@ class TestCF1_6(BaseTestCase):
         score, out_of, messages = get_results(results)
         assert score == out_of
 
+        dataset = MockTimeSeries()
+        temperature = dataset.createVariable("temperature", "f8", ("time",))
+        temperature.ancillary_variables = "temperature_flag"
+
+        temperature_flag = dataset.createVariable("temperature_flag", "i2", ("time",))
+        # bad modifier
+        temperature_flag.standard_name = "sea_water_temperature status flag"
+        _, _, messages = get_results(self.cf.check_standard_name(dataset))
+        assert ('Standard name modifier "status flag" for variable temperature_flag is not a valid modifier according to CF Appendix C'
+                in messages)
+        # proper name, units supplied
+        temperature_flag.standard_name = "sea_water_temperature status_flag"
+        temperature_flag.units = "1"
+        _, _, messages = get_results(self.cf.check_standard_name(dataset))
+
     def test_cell_bounds(self):
         dataset = self.load_dataset(STATIC_FILES["grid-boundaries"])
         results = self.cf.check_cell_boundaries(dataset)
@@ -1243,10 +1258,10 @@ class TestCF1_6(BaseTestCase):
         score, out_of, messages = get_results(results)
 
         msgs = [
-            u"Type of tempvalid_min attribute (int32) does not match variable type (int64)",
-            u"Type of temp:valid_max attribute (int32) does not match variable type (int64)",
-            u"Type of salinityvalid_min attribute (int32) does not match variable type (float64)",
-            u"Type of salinity:valid_max attribute (int32) does not match variable type (float64)",
+            "Type of tempvalid_min attribute (int32) does not match variable type (int64)",
+            "Type of temp:valid_max attribute (int32) does not match variable type (int64)",
+            "Type of salinityvalid_min attribute (int32) does not match variable type (float64)",
+            "Type of salinity:valid_max attribute (int32) does not match variable type (float64)",
         ]
 
         self.assertEqual(len(results), 4)
@@ -1311,10 +1326,38 @@ class TestCF1_6(BaseTestCase):
         #                 furthermore is fragile and breaks tests when check
         #                 definitions change
         scored, out_of, messages = get_results(results)
-        assert scored == 24
-        assert out_of == 24
+        assert scored == out_of
         assert messages == []
 
+    def test_check_standard_name_modifier_units(self):
+        """Test that standard name modifiers are properly processed"""
+        dataset = MockTimeSeries()
+
+        temp = dataset.createVariable("temperature", "f8", ("time",))
+        temp.standard_name = "sea_water_temperature"
+        temp.units = "degree_C"
+
+        temp_flag = dataset.createVariable("temp_flag", "i1", ("time",))
+        temp_flag.standard_name = "sea_water_temperature status_flag"
+        # units should not exist for
+        temp_flag.units = "1"
+
+        temp.ancillary_variables = "temp_flag"
+        scored, out_of, messages = get_results(self.cf.check_units(dataset))
+        assert scored != out_of
+        assert ("units attribute for variable temperature_flag must be unset "
+                "when status_flag modifier is set")
+
+        del temp_flag.units
+        scored, out_of, messages = get_results(self.cf.check_units(dataset))
+        assert scored == out_of
+
+        temp_counts = dataset.createVariable("temp_counts", "i1", ("time",))
+        temp.ancillary_variables += " temp_counts"
+
+
+
+
     def test_check_duplicates(self):
         """
         Test to verify that the check identifies duplicate axes. Load the
@@ -1883,6 +1926,18 @@ class TestCF1_7(BaseTestCase):
             result = self.cf.check_conventions_version(dataset)
             self.assertTrue(result.value)
 
+    def test_warn_on_deprecated_standard_name_modifiers(self):
+        ds = MockTimeSeries()
+        # normally, you'd set up a parent physical variable with
+        # ancillary_variables, but we only want to check bad standard name
+        # modifiers here
+        temp_counts = ds.createVariable("temp_counts", "i4", ("time",))
+        temp_counts.standard_name = "sea_water_temperature number_of_observations"
+        temp_flag = ds.createVariable("temp_flag", "i1", ("time",))
+        temp_flag.standard_name = "sea_water_temperature status_flag"
+        with pytest.warns(UserWarning):
+            self.cf.check_standard_name_deprecated_modifiers(ds)
+
     def test_appendix_d(self):
         """
         CF 1.7
@@ -2215,67 +2270,45 @@ class TestCF1_7(BaseTestCase):
         # set att bad (str)
         temp.setncattr("add_offset", "foo")
         r = self.cf.check_add_offset_scale_factor_type(dataset)
-        self.assertFalse(r[1].value)
+        self.assertFalse(r[0].value)
         # messages should be non-empty for improper type
-        self.assertTrue(r[1].msgs)
+        self.assertTrue(r[0].msgs)
         del temp.add_offset
 
         temp.setncattr("scale_factor", "foo")
         r = self.cf.check_add_offset_scale_factor_type(dataset)
-        self.assertFalse(r[1].value)
-        self.assertTrue(r[1].msgs)
+        self.assertFalse(r[0].value)
+        self.assertTrue(r[0].msgs)
 
         # set bad np val
         temp.setncattr("scale_factor", np.float32(5))
         r = self.cf.check_add_offset_scale_factor_type(dataset)
-        self.assertFalse(r[1].value)
-        self.assertTrue(r[1].msgs)
+        self.assertFalse(r[0].value)
+        self.assertTrue(r[0].msgs)
 
         temp.setncattr("scale_factor", np.uint(5))
         r = self.cf.check_add_offset_scale_factor_type(dataset)
-        self.assertFalse(r[1].value)
-        self.assertTrue(r[1].msgs)
+        self.assertFalse(r[0].value)
+        self.assertTrue(r[0].msgs)
 
         # set good
         temp.setncattr("scale_factor", np.float(5))
         r = self.cf.check_add_offset_scale_factor_type(dataset)
-        self.assertTrue(r[1].value)
-        self.assertFalse(r[1].msgs)
+        self.assertTrue(r[0].value)
+        self.assertFalse(r[0].msgs)
 
         temp.setncattr("scale_factor", np.double(5))
         r = self.cf.check_add_offset_scale_factor_type(dataset)
-        self.assertTrue(r[1].value)
-        self.assertFalse(r[1].msgs)
+        self.assertTrue(r[0].value)
+        self.assertFalse(r[0].msgs)
 
         # set same dtype
         dataset = MockTimeSeries()  # time lat lon depth
         temp = dataset.createVariable("temp", np.int, dimensions=("time",))
         temp.setncattr("scale_factor", np.int(5))
         r = self.cf.check_add_offset_scale_factor_type(dataset)
-        self.assertTrue(r[1].value)
-        self.assertFalse(r[1].msgs)
-
-        # integer variable type (int8, int16, int32) compared against
-        #floating point add_offset/scale_factor
-        for var_bytes in ("1", "2", "4"):
-            coarse_temp = dataset.createVariable(f"coarse_temp_{var_bytes}",
-                                                 f"i{var_bytes}",
-                                                 dimensions=("time",))
-            coarse_temp.setncattr("scale_factor", np.float32(23.0))
-            coarse_temp.setncattr("add_offset", np.double(-2.1))
-            r = self.cf.check_add_offset_scale_factor_type(dataset)
-            # First value which checks if add_offset and scale_factor
-            # are same type should be false
-            self.assertFalse(r[0].value)
-            self.assertEqual(r[0].msgs[0],
-                   "When both scale_factor and add_offset are supplied for "
-                   f"variable coarse_temp_{var_bytes}, they must have the "
-                   "same type")
-            # Individual checks for scale_factor/add_offset should be OK,
-            # however
-            self.assertTrue(r[-1].value)
-            self.assertFalse(r[-1].msgs)
-            del dataset.variables[f"coarse_temp_{var_bytes}"]
+        self.assertTrue(r[0].value)
+        self.assertFalse(r[0].msgs)
 
 
 class TestCF1_8(BaseTestCase):