Repository: ioos/compliance-checker
Pull request #1019 (2023-05-12 15:33:25)

Refactor cell_measures, properly handling quantity squared/cubed
Implements #953


Patch:
diff --git a/compliance_checker/cf/cf_1_6.py b/compliance_checker/cf/cf_1_6.py
index 7bb2a44..4ccbeca 100644
--- a/compliance_checker/cf/cf_1_6.py
+++ b/compliance_checker/cf/cf_1_6.py
@@ -2779,6 +2779,86 @@ class CF1_6Check(CFNCCheck):
             ret_val.append(result)
         return ret_val
 
+    def _cell_measures_core(self, ds, var, external_set, variable_template):
+        # IMPLEMENTATION CONFORMANCE REQUIRED 1/2
+        reasoning = []
+        search_str = (
+            r"^(?P<measure_type>area|volume):\s+(?P<cell_measure_var_name>\w+)$"
+        )
+        search_res = regex.match(search_str, var.cell_measures)
+        if not search_res:
+            valid = False
+            reasoning.append(
+                "The cell_measures attribute for variable {} "
+                "is formatted incorrectly. It should take the "
+                "form of either 'area: cell_var' or "
+                "'volume: cell_var' where cell_var is an existing name of "
+                "a variable describing the cell measures.".format(var.name)
+            )
+        else:
+            valid = True
+            cell_measure_var_name = search_res.group("cell_measure_var_name")
+            cell_measure_type = search_res.group("measure_type")
+            # TODO: cache previous results
+            if cell_measure_var_name not in set(ds.variables.keys()).union(
+                external_set
+            ):
+                valid = False
+                reasoning.append(
+                    f"Cell measure variable {cell_measure_var_name} referred to by "
+                    f"{var.name} is not present in {variable_template}s".format(
+                        cell_measure_var_name, var.name
+                    )
+                )
+            # CF 1.7+ assume external variables -- further checks can't be run here
+            elif cell_measure_var_name in external_set:
+                # can't test anything on an external var
+                return Result(
+                    BaseCheck.MEDIUM, valid, (self.section_titles["7.2"]), reasoning
+                )
+
+            else:
+                cell_measure_var = ds.variables[cell_measure_var_name]
+                if not hasattr(cell_measure_var, "units"):
+                    valid = False
+                    reasoning.append(
+                        "Cell measure variable {} is required "
+                        "to have units attribute defined".format(cell_measure_var_name)
+                    )
+                else:
+                    # IMPLEMENTATION CONFORMANCE REQUIRED 2/2
+                    # verify this combination {area: 'm2', volume: 'm3'}
+
+                    # key is valid measure types, value is expected
+                    # exponent
+                    exponent_lookup = {"area": 2, "volume": 3}
+                    exponent = exponent_lookup[search_res.group("measure_type")]
+                    conversion_failure_msg = (
+                        f'Variable "{cell_measure_var.name}" must have units which are convertible '
+                        f'to UDUNITS "m{exponent}" when variable is referred to by a {variable_template} with '
+                        f'cell_methods attribute with a measure type of "{cell_measure_type}".'
+                    )
+                    try:
+                        cell_measure_units = Unit(cell_measure_var.units)
+                    except ValueError:
+                        valid = False
+                        reasoning.append(conversion_failure_msg)
+                    else:
+                        if not cell_measure_units.is_convertible(Unit(f"m{exponent}")):
+                            valid = False
+                            reasoning.append(conversion_failure_msg)
+                    if not set(cell_measure_var.dimensions).issubset(var.dimensions):
+                        valid = False
+                        reasoning.append(
+                            "Cell measure variable {} must have "
+                            "dimensions which are a subset of "
+                            "those defined in variable {}.".format(
+                                cell_measure_var_name, var.name
+                            )
+                        )
+
+        return Result(BaseCheck.MEDIUM, valid, (self.section_titles["7.2"]), reasoning)
+
     def check_cell_measures(self, ds):
         """
         7.2 To indicate extra information about the spatial properties of a
@@ -2801,86 +2881,11 @@ class CF1_6Check(CFNCCheck):
         :return: List of results
         """
         ret_val = []
-        reasoning = []
         variables = ds.get_variables_by_attributes(
             cell_measures=lambda c: c is not None
         )
         for var in variables:
-            # IMPLEMENTATION CONFORMANCE REQUIRED 1/2
-            search_str = r"^(?:area|volume): (\w+)$"
-            search_res = regex.search(search_str, var.cell_measures)
-            if not search_res:
-                valid = False
-                reasoning.append(
-                    "The cell_measures attribute for variable {} "
-                    "is formatted incorrectly.  It should take the"
-                    " form of either 'area: cell_var' or "
-                    "'volume: cell_var' where cell_var is the "
-                    "variable describing the cell measures".format(var.name)
-                )
-            else:
-                valid = True
-                cell_meas_var_name = search_res.groups()[0]
-                # TODO: cache previous results
-                if cell_meas_var_name not in ds.variables:
-                    valid = False
-                    reasoning.append(
-                        "Cell measure variable {} referred to by "
-                        "{} is not present in dataset variables".format(
-                            cell_meas_var_name, var.name
-                        )
-                    )
-                else:
-                    cell_meas_var = ds.variables[cell_meas_var_name]
-                    if not hasattr(cell_meas_var, "units"):
-                        valid = False
-                        reasoning.append(
-                            "Cell measure variable {} is required "
-                            "to have units attribute defined.".format(
-                                cell_meas_var_name
-                            )
-                        )
-                    else:
-                        # IMPLEMENTATION CONFORMANCE REQUIRED 2/2
-                        # verify this combination {area: 'm2', volume: 'm3'}
-                        dic_expected = {"area": "m2", "volume": "m3"}
-                        dic_to_be_verified = {
-                            (var.cell_measures).split(":")[0]: cell_meas_var.units
-                        }
-
-                        if not set(dic_to_be_verified).issubset(dic_expected):
-                            valid = False
-                            reasoning.append(
-                                "Cell measure variable {} must have "
-                                "units that are consistent with the measure type."
-                                "i.e. {}.".format(cell_meas_var_name, dic_expected)
-                            )
-
-                        # verify units are recognized by UDUNITS
-                        valid_udunits = self._check_valid_udunits(
-                            ds, cell_meas_var_name
-                        )
-                        if valid_udunits.value[0] != valid_udunits.value[1]:
-                            valid = False
-                            reasoning.append(
-                                "Cell measure variable {} referred to by "
-                                "{} has a unit {} not recognized by UDUNITS".format(
-                                    cell_meas_var_name, var.name, cell_meas_var.units
-                                )
-                            )
-                    if not set(cell_meas_var.dimensions).issubset(var.dimensions):
-                        valid = False
-                        reasoning.append(
-                            "Cell measure variable {} must have "
-                            "dimensions which are a subset of "
-                            "those defined in variable {}.".format(
-                                cell_meas_var_name, var.name
-                            )
-                        )
-
-            result = Result(
-                BaseCheck.MEDIUM, valid, (self.section_titles["7.2"]), reasoning
-            )
+            result = self._cell_measures_core(ds, var, set(), "dataset variable")
             ret_val.append(result)
 
         return ret_val
diff --git a/compliance_checker/cf/cf_1_7.py b/compliance_checker/cf/cf_1_7.py
index a72b103..d527d16 100644
--- a/compliance_checker/cf/cf_1_7.py
+++ b/compliance_checker/cf/cf_1_7.py
@@ -5,7 +5,6 @@ from warnings import warn
 
 import numpy as np
 import pyproj
-import regex
 
 from compliance_checker import cfutil
 from compliance_checker.base import BaseCheck, Result, TestCtx
@@ -453,74 +452,26 @@ class CF1_7Check(CF1_6Check):
         :return: List of results
         """
         ret_val = []
-        reasoning = []
         variables = ds.get_variables_by_attributes(
             cell_measures=lambda c: c is not None
         )
+        try:
+            external_variables_str = ds.getncattr("external_variables")
+            if external_variables_str is not None:
+                external_variables_names = set(external_variables_str.split(" "))
+            else:
+                external_variables_names = set()
+        except (ValueError, AttributeError):
+            external_variables_names = set()
         for var in variables:
-            search_str = r"^(?:area|volume): (\w+)$"
-            search_res = regex.search(search_str, var.cell_measures)
-            if not search_res:
-                valid = False
-                reasoning.append(
-                    "The cell_measures attribute for variable {} "
-                    "is formatted incorrectly.  It should take the"
-                    " form of either 'area: cell_var' or "
-                    "'volume: cell_var' where cell_var is the "
-                    "variable describing the cell measures".format(var.name)
+            ret_val.append(
+                self._cell_measures_core(
+                    ds,
+                    var,
+                    external_variables_names,
+                    "dataset or external variable",
                 )
-            else:
-                valid = True
-                cell_meas_var_name = search_res.groups()[0]
-                # TODO: cache previous results
-
-                # if the dataset has external_variables, get it
-                try:
-                    external_variables = ds.getncattr("external_variables")
-                except AttributeError:
-                    external_variables = []
-                if cell_meas_var_name not in ds.variables:
-                    if cell_meas_var_name not in external_variables:
-                        valid = False
-                        reasoning.append(
-                            "Cell measure variable {} referred to by {} is not present in dataset variables".format(
-                                cell_meas_var_name, var.name
-                            )
-                        )
-                    else:
-                        valid = True
-
-                    # make Result
-                    result = Result(
-                        BaseCheck.MEDIUM, valid, (self.section_titles["7.2"]), reasoning
-                    )
-                    ret_val.append(result)
-                    continue  # can't test anything on an external var
-
-                else:
-                    cell_meas_var = ds.variables[cell_meas_var_name]
-                    if not hasattr(cell_meas_var, "units"):
-                        valid = False
-                        reasoning.append(
-                            "Cell measure variable {} is required "
-                            "to have units attribute defined.".format(
-                                cell_meas_var_name
-                            )
-                        )
-                    if not set(cell_meas_var.dimensions).issubset(var.dimensions):
-                        valid = False
-                        reasoning.append(
-                            "Cell measure variable {} must have "
-                            "dimensions which are a subset of "
-                            "those defined in variable {}.".format(
-                                cell_meas_var_name, var.name
-                            )
-                        )
-
-            result = Result(
-                BaseCheck.MEDIUM, valid, (self.section_titles["7.2"]), reasoning
             )
-            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 5419285..eda75db 100644
--- a/compliance_checker/cf/cf_1_9.py
+++ b/compliance_checker/cf/cf_1_9.py
@@ -104,6 +104,7 @@ class CF1_9Check(CF1_8Check):
             for var in ds.get_variables_by_attributes(
                 coordinates=lambda c: c is not None
             )
+            # IMPLICIT
             if not var.dimensions
         ):
             domain_valid = TestCtx(BaseCheck.MEDIUM, self.section_titles["5.8"])


Test patch:
diff --git a/compliance_checker/tests/test_cf.py b/compliance_checker/tests/test_cf.py
index 3fa18b6..f762d8f 100644
--- a/compliance_checker/tests/test_cf.py
+++ b/compliance_checker/tests/test_cf.py
@@ -648,12 +648,13 @@ class TestCF1_6(BaseTestCase):
         dataset = self.load_dataset(STATIC_FILES["bad_cell_measure1"])
         results = self.cf.check_cell_measures(dataset)
         score, out_of, messages = get_results(results)
-        message = (
-            "The cell_measures attribute for variable PS is formatted incorrectly.  "
+        expected_message = (
+            "The cell_measures attribute for variable PS is formatted incorrectly. "
             "It should take the form of either 'area: cell_var' or 'volume: cell_var' "
-            "where cell_var is the variable describing the cell measures"
+            "where cell_var is an existing name of a variable describing the "
+            "cell measures."
         )
-        assert message in messages
+        assert expected_message in messages
 
         dataset = self.load_dataset(STATIC_FILES["bad_cell_measure2"])
         results = self.cf.check_cell_measures(dataset)
@@ -661,6 +662,42 @@ class TestCF1_6(BaseTestCase):
         message = "Cell measure variable box_area referred to by PS is not present in dataset variables"
         assert message in messages
 
+        dataset = MockTimeSeries()
+        dataset.createVariable("PS", "d", ("time",))  # dtype=double, dims=time
+        dataset.variables["PS"].setncattr("cell_measures", "area: cell_area")
+        # ensure the cell_measures var is in the dataset
+        dataset.createVariable("cell_area", "d", ("time",))
+        dataset.variables["cell_area"].setncattr("units", "m3")
+        # TEST CONFORMANCE 7.2 REQUIRED
+        # inappropriate length exponent for area
+        expected_fail_msg = (
+            'Variable "cell_area" must have units which are convertible '
+            'to UDUNITS "m2" when variable is referred to by a dataset variable with '
+            'cell_methods attribute with a measure type of "area".'
+        )
+        results = self.cf.check_cell_measures(dataset)
+        score, out_of, messages = get_results(results)
+        assert expected_fail_msg in messages
+
+        # set erroneous units that aren't convertible to UDUnits length
+        # units
+        dataset.variables["cell_area"].setncattr("units", "s3")
+        results = self.cf.check_cell_measures(dataset)
+        score, out_of, messages = get_results(results)
+        assert expected_fail_msg in messages
+
+        # TEST CONFORMANCE 7.2 REQUIRED 1/2
+        dataset.createDimension("depth2", 5)
+        dataset.variables["PS"].setncattr("cell_measures", "area: cell_area2")
+        dataset.createVariable("cell_area2", "f8", ("time", "depth2"))
+        dataset.variables["cell_area2"].setncattr("units", "m2")
+        results = self.cf.check_cell_measures(dataset)
+        score, out_of, messages = get_results(results)
+        assert (
+            "Cell measure variable cell_area2 must have dimensions which are a subset of those defined in variable PS."
+            in messages
+        )
+
     def test_climatology_cell_methods(self):
         """
         Checks that climatology cell_methods strings are properly validated
@@ -706,7 +743,7 @@ class TestCF1_6(BaseTestCase):
         score, out_of, messages = get_results(results)
         self.assertEqual(score, out_of)
 
-        # TEST CONFORMMANCE 7.4 REQUIRED 5/6
+        # TEST CONFORMANCE 7.4 REQUIRED 5/6
         dataset.variables["climatology_bounds"] = MockVariable(
             dataset.variables["climatology_bounds"]
         )
@@ -2094,8 +2131,6 @@ class TestCF1_7(BaseTestCase):
         assert (score, out_of) == (1, 2)
 
     def test_cell_measures(self):
-        """Over-ride the test_cell_measures from CF1_6"""
-
         # create a temporary variable and test this only
         with MockTimeSeries() as dataset:
             dataset.createVariable("PS", "d", ("time",))  # dtype=double, dims=time
@@ -2109,37 +2144,72 @@ class TestCF1_7(BaseTestCase):
             score, out_of, messages = get_results(results)
             assert (score == out_of) and (score > 0)
 
-        # same thing, but test that the cell_area variable is in
-        # the global attr "external_variables"
+            # bad measure, not area or volume
+            dataset.variables["PS"].cell_measures = "length: cell_area"
+            results = self.cf.check_cell_measures(dataset)
+            score, out_of, messages = get_results(results)
+            assert (
+                "The cell_measures attribute for variable PS is formatted "
+                "incorrectly. It should take the form of either 'area: "
+                "cell_var' or 'volume: cell_var' where cell_var is an "
+                "existing name of a variable describing the cell measures." in messages
+            )
 
-        with MockTimeSeries() as dataset:
-            dataset.createVariable("PS", "d", ("time",))  # dtype=double, dims=time
-            dataset.variables["PS"].setncattr("cell_measures", "area: cell_area")
-            dataset.setncattr("external_variables", ["cell_area"])
+            # proper measure type, but referenced variable does not exist
+            dataset.variables["PS"].cell_measures = "area: NONEXISTENT_VAR"
+            results = self.cf.check_cell_measures(dataset)
+            score, out_of, messages = get_results(results)
+            assert (
+                "Cell measure variable NONEXISTENT_VAR referred to by "
+                "PS is not present in dataset or external variables" in messages
+            )
 
-            # run the check
+            dataset.variables["PS"].cell_measures = "area: no_units"
+            dataset.createVariable("no_units", "i2", ())
             results = self.cf.check_cell_measures(dataset)
             score, out_of, messages = get_results(results)
-            assert score > 0
-            assert score == out_of
+            assert (
+                "Cell measure variable no_units is required to have units "
+                "attribute defined" in messages
+            )
+
+        # cell_area variable is in
+        # the global attr "external_variables"
+
+        dataset = MockTimeSeries()
+        dataset.createVariable("PS", "d", ("time",))  # dtype=double, dims=time
+        dataset.variables["PS"].setncattr("cell_measures", "area: cell_area")
+        dataset.setncattr("external_variables", "cell_area")
+
+        # run the check
+        results = self.cf.check_cell_measures(dataset)
+        score, out_of, messages = get_results(results)
+        assert score > 0
+        assert score == out_of
+
+        # Non-string external variables, just treat as empty
+        dataset.setncattr("external_variables", 1)
+        results = self.cf.check_cell_measures(dataset)
+        score, out_of, messages = get_results(results)
+        message = "Cell measure variable cell_area referred to by PS is not present in dataset or external variables"
 
         # now test a dataset with a poorly formatted cell_measure attr
         dataset = self.load_dataset(STATIC_FILES["bad_cell_measure1"])
         results = self.cf.check_cell_measures(dataset)
         score, out_of, messages = get_results(results)
-        message = (
-            "The cell_measures attribute for variable PS is formatted incorrectly.  "
+        expected_message = (
+            "The cell_measures attribute for variable PS is formatted incorrectly. "
             "It should take the form of either 'area: cell_var' or 'volume: cell_var' "
-            "where cell_var is the variable describing the cell measures"
+            "where cell_var is an existing name of a variable describing the cell measures."
         )
-        assert message in messages
+        assert expected_message in messages
 
         # test a dataset where the cell_measure attr is not in the dataset or external_variables
         # check for the variable should fail
         dataset = self.load_dataset(STATIC_FILES["bad_cell_measure2"])
         results = self.cf.check_cell_measures(dataset)
         score, out_of, messages = get_results(results)
-        message = "Cell measure variable box_area referred to by PS is not present in dataset variables"
+        message = "Cell measure variable box_area referred to by PS is not present in dataset or external variables"
         assert message in messages
 
     def test_variable_features(self):