Repository: ioos/compliance-checker
Pull request #496 (2017-05-16 15:49:39)

Fixes cell_measures checker bug
Fixes https://github.com/ioos/compliance-checker/issues/495

This pull-request fixes the issues brought up in #495 and also adds _proper_ unit tests that actually test the check.


Patch:
diff --git a/compliance_checker/cf/cf.py b/compliance_checker/cf/cf.py
index baa8682..ec44425 100644
--- a/compliance_checker/cf/cf.py
+++ b/compliance_checker/cf/cf.py
@@ -2560,15 +2560,20 @@ class CFBaseCheck(BaseCheck):
 
     def check_cell_measures(self, ds):
         """
-        7.2 To indicate extra information about the spatial properties of a variable's grid cells, a cell_measures attribute may
-        be defined for a variable. This is a string attribute comprising a list of blank-separated pairs of words of the form
-        "measure: name". "area" and "volume" are the only defined measures.
+        7.2 To indicate extra information about the spatial properties of a
+        variable's grid cells, a cell_measures attribute may be defined for a
+        variable. This is a string attribute comprising a list of
+        blank-separated pairs of words of the form "measure: name". "area" and
+        "volume" are the only defined measures.
 
-        The "name" is the name of the variable containing the measure values, which we refer to as a "measure variable". The
-        dimensions of the measure variable should be the same as or a subset of the dimensions of the variable to which they are
-        related, but their order is not restricted.
+        The "name" is the name of the variable containing the measure values,
+        which we refer to as a "measure variable". The dimensions of the
+        measure variable should be the same as or a subset of the dimensions of
+        the variable to which they are related, but their order is not
+        restricted.
 
-        The variable must have a units attribute and may have other attributes such as a standard_name.
+        The variable must have a units attribute and may have other attributes
+        such as a standard_name.
 
         :param netCDF4.Dataset ds: An open netCDF dataset
         :rtype: list
@@ -2576,10 +2581,9 @@ class CFBaseCheck(BaseCheck):
         """
         ret_val = []
         reasoning = []
-        var_names = ds.get_variables_by_attributes(cell_measures=lambda c:
+        variables = ds.get_variables_by_attributes(cell_measures=lambda c:
                                                    c is not None)
-        for var_name in var_names:
-            var = ds.variables[var_name]
+        for var in variables:
             search_str = '^(?:area|volume): (\w+)$'
             search_res = re.search(search_str, var.cell_measures)
             if not search_res:
@@ -2589,41 +2593,41 @@ class CFBaseCheck(BaseCheck):
                                  " form of either 'area: cell_var' or "
                                  "'volume: cell_var' where cell_var is the "
                                  "variable describing the cell measures".format(
-                                     var_name))
+                                     var.name))
+                continue
+            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(
+                        var.name, cell_meas_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:
+                cell_meas_var = ds.variables[cell_meas_var_name]
+                if not hasattr(cell_meas_var, 'units'):
                     valid = False
                     reasoning.append(
-                        "Cell measure variable {} referred to by "
-                        "{} is not present in dataset variables".format(
-                            var_name, cell_meas_var_name)
+                        "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)
                     )
-                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,
-                            ('§7.2 Cell measures', var_name, 'cell_measures'),
-                            reasoning)
-            ret_val.append(result)
+        result = Result(BaseCheck.MEDIUM,
+                        valid,
+                        ('§7.2 Cell measures', var.name, 'cell_measures'),
+                        reasoning)
+        ret_val.append(result)
 
         return ret_val
 


Test patch:
diff --git a/compliance_checker/tests/data/bad_cell_measure1.cdl b/compliance_checker/tests/data/bad_cell_measure1.cdl
new file mode 100644
index 0000000..58ebb36
--- /dev/null
+++ b/compliance_checker/tests/data/bad_cell_measure1.cdl
@@ -0,0 +1,29 @@
+netcdf bad_cell_measure1 {
+// This dataset uses an invalid measurement for cell_measures
+dimensions:
+    cell = 4;
+    time = 2;
+    nv = 4;
+variables:
+    float PS(time, cell);
+        PS:units = "Pa";
+        PS:coordinates = "lon lat";
+        PS:cell_measures = "box: cell_area";
+    float lon(cell);
+        lon:standard_name = "longitude";
+        lon:units = "degrees_east";
+    float lat(cell);
+        lat:standard_name = "latitude";
+        lat:units = "degrees_north";
+    float time(time);
+        time:standard_name = "time";
+        time:units = "seconds since 1970-01-01";
+    float cell_area(cell);
+        cell_area:long_name = "area of grid cell";
+        cell_area:standard_name = "area";
+        cell_area:units = "m2";
+    float lon_vertices(cell, nv);
+    float lat_vertices(cell, nv);
+}
+
+
diff --git a/compliance_checker/tests/data/bad_cell_measure2.cdl b/compliance_checker/tests/data/bad_cell_measure2.cdl
new file mode 100644
index 0000000..a25e723
--- /dev/null
+++ b/compliance_checker/tests/data/bad_cell_measure2.cdl
@@ -0,0 +1,30 @@
+netcdf bad_cell_measure2 {
+// This dataset references a non-existant variable for cell_measures
+dimensions:
+    cell = 4;
+    time = 2;
+    nv = 4;
+variables:
+    float PS(time, cell);
+        PS:units = "Pa";
+        PS:coordinates = "lon lat";
+        PS:cell_measures = "area: box_area";
+    float lon(cell);
+        lon:standard_name = "longitude";
+        lon:units = "degrees_east";
+    float lat(cell);
+        lat:standard_name = "latitude";
+        lat:units = "degrees_north";
+    float time(time);
+        time:standard_name = "time";
+        time:units = "seconds since 1970-01-01";
+    float cell_area(cell);
+        cell_area:long_name = "area of grid cell";
+        cell_area:standard_name = "area";
+        cell_area:units = "m2";
+    float lon_vertices(cell, nv);
+    float lat_vertices(cell, nv);
+}
+
+
+
diff --git a/compliance_checker/tests/data/cell_measure.cdl b/compliance_checker/tests/data/cell_measure.cdl
new file mode 100644
index 0000000..4125a7f
--- /dev/null
+++ b/compliance_checker/tests/data/cell_measure.cdl
@@ -0,0 +1,27 @@
+netcdf cell_measure {
+dimensions:
+    cell = 4;
+    time = 2;
+    nv = 4;
+variables:
+    float PS(time, cell);
+        PS:units = "Pa";
+        PS:coordinates = "lon lat";
+        PS:cell_measures = "area: cell_area";
+    float lon(cell);
+        lon:standard_name = "longitude";
+        lon:units = "degrees_east";
+    float lat(cell);
+        lat:standard_name = "latitude";
+        lat:units = "degrees_north";
+    float time(time);
+        time:standard_name = "time";
+        time:units = "seconds since 1970-01-01";
+    float cell_area(cell);
+        cell_area:long_name = "area of grid cell";
+        cell_area:standard_name = "area";
+        cell_area:units = "m2";
+    float lon_vertices(cell, nv);
+    float lat_vertices(cell, nv);
+}
+
diff --git a/compliance_checker/tests/resources.py b/compliance_checker/tests/resources.py
index 05d244b..5c47f5a 100644
--- a/compliance_checker/tests/resources.py
+++ b/compliance_checker/tests/resources.py
@@ -107,5 +107,8 @@ STATIC_FILES = {
     'fvcom'                                : get_filename('tests/data/examples/fvcom.cdl'),
     'ww3'                                  : get_filename('tests/data/examples/ww3.cdl'),
     'glcfs'                                : get_filename('tests/data/examples/glcfs.cdl'),
-    'NCEI_profile_template_v2_0'           : get_filename('tests/data/NCEI_profile_template_v2.0_2016-09-22_181835.151325.cdl')
+    'NCEI_profile_template_v2_0'           : get_filename('tests/data/NCEI_profile_template_v2.0_2016-09-22_181835.151325.cdl'),
+    'cell_measure'                         : get_filename('tests/data/cell_measure.cdl'),
+    'bad_cell_measure1'                    : get_filename('tests/data/bad_cell_measure1.cdl'),
+    'bad_cell_measure2'                    : get_filename('tests/data/bad_cell_measure2.cdl'),
 }
diff --git a/compliance_checker/tests/test_cf.py b/compliance_checker/tests/test_cf.py
index 53fcff8..8f00294 100644
--- a/compliance_checker/tests/test_cf.py
+++ b/compliance_checker/tests/test_cf.py
@@ -348,9 +348,25 @@ class TestCF(BaseTestCase):
         assert u"Boundary variable dimension lon_bnds must have at least 2 elements to form a simplex/closed cell with previous dimensions {}.".format(tuple_format) in messages
 
     def test_cell_measures(self):
-        dataset = self.load_dataset(STATIC_FILES['cf_example_cell_measures'])
-        results = self.cf.check_climatological_statistics(dataset)
+        dataset = self.load_dataset(STATIC_FILES['cell_measure'])
+        results = self.cf.check_cell_measures(dataset)
+        score, out_of, messages = self.get_results(results)
+        assert score == out_of
+        assert score > 0
+
+        dataset = self.load_dataset(STATIC_FILES['bad_cell_measure1'])
+        results = self.cf.check_cell_measures(dataset)
+        score, out_of, messages = self.get_results(results)
+        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")
+        assert message in messages
+
+        dataset = self.load_dataset(STATIC_FILES['bad_cell_measure2'])
+        results = self.cf.check_cell_measures(dataset)
         score, out_of, messages = self.get_results(results)
+        message = 'Cell measure variable PS referred to by box_area is not present in dataset variables'
+        assert message in messages
 
     def test_climatology(self):
         dataset = self.load_dataset(STATIC_FILES['climatology'])