Repository: ioos/compliance-checker
Pull request #961 (2022-10-04 21:54:29)

Only allow fixed length string variables to be one or two-dimensional
Implements missing point of #960 1/2 .


Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index a29bcfc..613a198 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -13,7 +13,7 @@ repos:
     - id: requirements-txt-fixer
 
 - repo: https://github.com/PyCQA/flake8
-  rev: 5.0.4
+  rev: 4.0.1
   hooks:
     - id: flake8
       exclude: docs/source/conf.py
@@ -27,13 +27,13 @@ repos:
     args: ["--profile", "black", "--filter-files"]
 
 - repo: https://github.com/psf/black
-  rev: 22.8.0
+  rev: 22.6.0
   hooks:
   - id: black
     language_version: python3
 
 - repo: https://github.com/codespell-project/codespell
-  rev: v2.2.1
+  rev: v2.1.0
   hooks:
     - id: codespell
       args:
diff --git a/compliance_checker/cf/cf_1_6.py b/compliance_checker/cf/cf_1_6.py
index b31519a..95ff09b 100644
--- a/compliance_checker/cf/cf_1_6.py
+++ b/compliance_checker/cf/cf_1_6.py
@@ -94,6 +94,13 @@ class CF1_6Check(CFNCCheck):
                         k, v.datatype
                     )
                 )
+            # TODO: DRY, don't repeat code path
+            if v.dtype is not str and v.dtype.kind == "S" and v.ndim not in (1, 2):
+                fails.append(
+                    "The fixed-length string variable {} must be one or "
+                    "two-dimensional, current length is {}".format(v.name, v.ndim)
+                )
+
         return Result(
             BaseCheck.HIGH,
             (total - len(fails), total),
@@ -155,7 +162,8 @@ class CF1_6Check(CFNCCheck):
                 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.int32, int)
+                and variable.dtype
+                in (np.byte, np.short, np.int16, np.int, np.int32, int)
             )
         if not val:
             msgs.append(error_msg)
@@ -3155,6 +3163,7 @@ class CF1_6Check(CFNCCheck):
                 else:
                     # Check variable type is byte, short or int
                     if var.dtype.type not in [
+                        np.int,
                         np.int8,
                         np.int16,
                         np.int32,
diff --git a/compliance_checker/cf/cf_1_8.py b/compliance_checker/cf/cf_1_8.py
index 2aa643b..fc666cc 100644
--- a/compliance_checker/cf/cf_1_8.py
+++ b/compliance_checker/cf/cf_1_8.py
@@ -484,7 +484,7 @@ class GeometryStorage(object):
     def check_geometry(self):
         invalid_vars = []
         for coord_var in self.coord_vars:
-            if not np.issubdtype(coord_var, float):
+            if not np.issubdtype(coord_var, np.float):
                 invalid_vars.append(coord_var.name)
         # can't continue if the geometry variables are not the correct size
         if invalid_vars:


Test patch:
diff --git a/compliance_checker/tests/test_cf.py b/compliance_checker/tests/test_cf.py
index e3e3ac6..d5ea42a 100644
--- a/compliance_checker/tests/test_cf.py
+++ b/compliance_checker/tests/test_cf.py
@@ -106,7 +106,10 @@ class TestCF1_6(BaseTestCase):
         ds.createDimension("siglev", 20)
 
         temp = ds.createVariable(
-            "temp", np.float64, dimensions=("time",), fill_value=99999999999999999999.0
+            "temp",
+            np.float64,
+            dimensions=("time",),
+            fill_value=np.float(99999999999999999999.0),
         )
         temp.coordinates = "sigma noexist"
         ds.createVariable("sigma", np.float64, dimensions=("siglev",))
@@ -150,6 +153,35 @@ class TestCF1_6(BaseTestCase):
         dataset = self.load_dataset(STATIC_FILES["bad_data_type"])
         result = self.cf.check_data_types(dataset)
 
+        dataset = MockTimeSeries()
+        # test dimensionality of string
+        dataset.createDimension("num_strings", 3)
+        dataset.createDimension("max_string_length", 80)
+        dataset.createDimension("extra", 9)
+
+        # TEST CONFORMANCE 2.2 1/2
+        # create strings with no dimensions -- fail
+        dataset.createVariable("zero_dimensional", "S1", ())
+        # create strings with one dimension -- valid
+        dataset.createVariable("one_dimensional", "S1", ("max_string_length"))
+        # create strings with two dimensions - valid
+        # NB: string length is supposed to be last, but this can't be inferred
+        #     easily with
+        dataset.createVariable(
+            "two_dimensional", "S1", ("num_strings", "max_string_length")
+        )
+        # create strings with three dimensions - valid
+        dataset.createVariable(
+            "three_dimensional", "S1", ("num_strings", "max_string_length", "extra")
+        )
+
+        result = self.cf.check_data_types(dataset)
+        expected_msgs = [
+            f"The fixed-length string variable {dim_name}_dimensional must "
+            f"be one or two-dimensional, current length is {dim_len}"
+            for dim_name, dim_len in (("zero", 0), ("three", 3))
+        ]
+        assert result.msgs == expected_msgs
         # TODO
         # the acdd_reformat_rebase branch has a new .nc file
         # which constructs the temp variable with an int64 dtype --
@@ -181,7 +213,7 @@ class TestCF1_6(BaseTestCase):
             "temp",
             np.float64,
             dimensions=("time",),
-            fill_value=np.float64(99999999999999999999.0),
+            fill_value=np.float(99999999999999999999.0),
         )
 
         # give temp _FillValue as a float, expect good result
@@ -1194,7 +1226,7 @@ class TestCF1_6(BaseTestCase):
         assert bad_month_msg in messages
 
         dataset.variables["time"].month_lengths = np.array(
-            [31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31], dtype=np.uint8
+            [31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31], dtype=np.int
         )
         results = self.cf.check_calendar(dataset)
         scored, out_of, messages = get_results(results)
@@ -2265,47 +2297,40 @@ class TestCF1_7(BaseTestCase):
         # create a temporary variable and test this only
         nc_obj = MockTimeSeries()
         nc_obj.createVariable("temperature", "d", ("time",))
+        nc_obj.variables["temperature"].setncattr("test_att", np.float64(45))
         att_name = "test_att"
         _var = nc_obj.variables["temperature"]
 
         # first, test all valid checks show that it's valid
-        _var.test_att = "my_attr_value"  # string
+        attr = "my_attr_value"  # string
         attr_type = "S"
-        result = self.cf._check_attr_type(att_name, attr_type, _var.test_att)
+        result = self.cf._check_attr_type(att_name, attr_type, attr)
         self.assertTrue(result[0])
 
-        _var.test_att = np.int8(1)
+        attr = np.int64(1)
         attr_type = "N"
-        self.assertTrue(self.cf._check_attr_type(att_name, attr_type, _var.test_att)[0])
+        self.assertTrue(self.cf._check_attr_type(att_name, attr_type, attr)[0])
 
-        _var.test_att = np.float64(45)
+        attr = np.float64(45)
         attr_type = "D"
-        self.assertTrue(
-            self.cf._check_attr_type(att_name, attr_type, _var.test_att, _var)[0]
-        )
+        self.assertTrue(self.cf._check_attr_type(att_name, attr_type, attr, _var)[0])
 
         # check failures
-        _var.test_att = "my_attr_value"
+        attr = "my_attr_value"
         attr_type = "N"  # should be numeric
-        self.assertFalse(
-            self.cf._check_attr_type(att_name, attr_type, _var.test_att)[0]
-        )
+        self.assertFalse(self.cf._check_attr_type(att_name, attr_type, attr)[0])
 
-        _var.test_att = np.int8(64)
+        attr = np.int(64)
         attr_type = "S"  # should be string
-        self.assertFalse(
-            self.cf._check_attr_type(att_name, attr_type, _var.test_att)[0]
-        )
+        self.assertFalse(self.cf._check_attr_type(att_name, attr_type, attr)[0])
 
         nc_obj = MockTimeSeries()
         nc_obj.createVariable("temperature", "d", ("time",))
-        nc_obj.variables["temperature"].setncattr("test_att", 45)
+        nc_obj.variables["temperature"].setncattr("test_att", np.int32(45))
         _var = nc_obj.variables["temperature"]
-        _var.test_att = np.int8(45)
+        attr = np.int32(45)
         attr_type = "D"  # should match
-        self.assertFalse(
-            self.cf._check_attr_type(att_name, attr_type, _var.test_att, _var)[0]
-        )
+        self.assertFalse(self.cf._check_attr_type(att_name, attr_type, attr, _var)[0])
 
     def test_check_grid_mapping_attr_condition(self):
         """
@@ -2508,13 +2533,13 @@ class TestCF1_7(BaseTestCase):
         self.assertFalse(r[1].value)
         self.assertTrue(r[1].msgs)
 
-        temp.setncattr("scale_factor", np.uint32(5))
+        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)
 
         # set good
-        temp.setncattr("scale_factor", np.float64(5))
+        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)
@@ -2526,8 +2551,8 @@ class TestCF1_7(BaseTestCase):
 
         # set same dtype
         dataset = MockTimeSeries()  # time lat lon depth
-        temp = dataset.createVariable("temp", np.int32, dimensions=("time",))
-        temp.setncattr("scale_factor", np.int32(5))
+        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)