Repository: ioos/compliance-checker
Pull request #765 (2020-03-13 19:34:18)

Ensure platform_id is an alphanumeric string
This addresses #762 quite succinctly, but I'm worried that the error message at 

https://github.com/ioos/compliance-checker/blob/54b5300663b70e586ca429e2ecdad12f0e155314/compliance_checker/base.py#L488

is going to be quite unclear to some users. I'm going to ping for @benjwadams for some insight. The machinery is already here, which makes a pull request this simple so tempting.


Patch:
diff --git a/compliance_checker/acdd.py b/compliance_checker/acdd.py
index 14925f6..f5e231a 100644
--- a/compliance_checker/acdd.py
+++ b/compliance_checker/acdd.py
@@ -6,7 +6,7 @@ project for the verification and scoring of attributes for datasets.
 '''
 
 import numpy as np
-from netCDF4 import num2date
+from cftime import num2pydate
 from datetime import timedelta
 from compliance_checker.base import (BaseCheck, BaseNCCheck, check_has,
                                      Result, ratable_result)
@@ -536,9 +536,9 @@ class ACDDBaseCheck(BaseCheck):
             # subtraction from t_min/t_max will assume that a naive timestamp is
             # in the same time zone and cause erroneous results.
             # Pendulum uses UTC by default, but we are being explicit here
-            time0 = pendulum.instance(num2date(ds.variables[timevar][0],
+            time0 = pendulum.instance(num2pydate(ds.variables[timevar][0],
                                       ds.variables[timevar].units), 'UTC')
-            time1 = pendulum.instance(num2date(ds.variables[timevar][-1],
+            time1 = pendulum.instance(num2pydate(ds.variables[timevar][-1],
                                       ds.variables[timevar].units), 'UTC')
         except:
             return Result(BaseCheck.MEDIUM,
diff --git a/compliance_checker/ioos.py b/compliance_checker/ioos.py
index 5e94ed1..5414a3b 100644
--- a/compliance_checker/ioos.py
+++ b/compliance_checker/ioos.py
@@ -11,6 +11,7 @@ from compliance_checker.cfutil import (get_geophysical_variables,
                                        get_instrument_variables,
                                        get_coordinate_variables)
 from compliance_checker import base
+from compliance_checker.cf import util as cf_util # not to be confused with cfutil.py
 from compliance_checker.cf.cf import CF1_6Check, CF1_7Check
 import validators
 import re
@@ -358,6 +359,10 @@ class IOOS1_2_ConventionsValidator(base.RegexValidator):
     validator_regex = r"\bIOOS-1.2\b"
     validator_fail_msg = "{} must contain the string \"IOOS 1.2\""
 
+class IOOS1_2_PlatformIDValidator(base.RegexValidator):
+    validator_regex = r"^[a-zA-Z0-9]+$"
+    validator_fail_msg = "{} must be alphanumeric"
+
 class NamingAuthorityValidator(base.UrlValidator):
     """
     Class to check for URL or reversed DNS strings contained within
@@ -412,7 +417,7 @@ class IOOS1_2Check(IOOSNCCheck):
 
         # geophysical variables must have the following attrs:
         self.geophys_check_var_attrs = self._default_check_var_attrs.union(set([
-            ("standard_name_uri", BaseCheck.MEDIUM),
+            ("standard_name_url", BaseCheck.MEDIUM),
             #( "platform", BaseCheck.HIGH) # checked under check_single_platform()
             #( "wmo_platform_code", BaseCheck.HIGH) # only "if applicable", see check_wmo_platform_code()
             #( "ancillary_variables", BaseCheck.HIGH) # only "if applicable", see _check_var_gts_ingest() 
@@ -451,8 +456,6 @@ class IOOS1_2Check(IOOSNCCheck):
         self.rec_atts = [
             ('contributor_email', base.EmailValidator()),
             'contributor_name',
-            'contributor_role',
-            'contributor_role_vocabulary',
             ('contributor_url', base.UrlValidator(base.csv_splitter)),
             'creator_address',
             'creator_city',
@@ -464,9 +467,8 @@ class IOOS1_2Check(IOOSNCCheck):
             #'creator_type',
             'institution',
             'instrument',
-            'ioos_ingest',
             'keywords',
-            'platform_id',
+            ('platform_id', IOOS1_2_PlatformIDValidator()), # alphanumeric only
             'publisher_address',
             'publisher_city',
             'publisher_name',
@@ -888,89 +890,155 @@ class IOOS1_2Check(IOOSNCCheck):
         val = bool(validators.url(pvocab))
         return Result(BaseCheck.MEDIUM, val, "platform_vocabulary", [m])
             
-    def _check_var_gts_ingest(self, ds, var, do_ingest, msg):
+    def _check_gts_ingest_val(self, val):
+        """
+        Check that `val` is a str and is equal to "true" or "false"
+
+        Parmeters
+        ---------
+        val (?): value to check
+
+        Returns
+        -------
+        bool
+        """
+
+        return isinstance(val, str) and val.lower() in {"true", "false"}
+
+
+    def check_gts_ingest_global(self, ds):
+        """
+        Check if a dataset has the global attribute "gts_ingest" and that
+        it matches "true" or "false". This attribute is "required, if applicable".
+
+        Parameters
+        ----------
+        ds (netCDF4.Dataset): open dataset
+
+        Returns
+        -------
+        Result
+        """
+
+        r = True # default
+        gts = getattr(ds, "gts_ingest", None)
+
+        if gts:
+            r = self._check_gts_ingest_val(gts)
+
+        return Result(BaseCheck.HIGH, r, "gts_ingest", ["gts_ingest must be a string \"true\" or \"false\""])
+
+    def _var_qualifies_for_gts_ingest(self, ds, var):
         """
-        Helper function for check_gts_ingest(). Check that a given variable
+        Examine a variable to see if it qualifies for GTS Ingest.
+        Check that a given variable
           - has a valid CF standard name (checked with check_standard_names())
           - has a QARTOD aggregates variable
           - has valid units (checked with check_units())
 
-        Args:
-            attr (?): attribute value
+        Parameters
+        ----------
+        ds (netCDF4.Dataset): open Dataset
+        var (netCDF4.Variable): variable from dataset
 
-        Returns:
-            Result
+        Returns
+        -------
+        bool
         """
 
         val = False
 
         # should have an ancillary variable with standard_name aggregate_quality_flag
         avar_val = False
-        anc_vars = getattr(var, "ancillary_variables", "").split(" ")
+        anc_vars = str(getattr(var, "ancillary_variables", "")).split(" ")
         for av in anc_vars:
             if av in ds.variables:
                 if getattr(ds.variables[av], "standard_name", "") == "aggregate_quality_flag":
                     avar_val = True
                     break
 
-        # if variable is flagged for ingest, but no global attr present, error
-        val = True if (avar_val and do_ingest) else False
-
-        return Result(BaseCheck.HIGH, val, "gts_ingest variable", [msg])
-
-    def check_gts_ingest(self, ds):
+        # should have compliant standard_name
+        # NOTE: standard names are checked extensively in self.check_standard_names()
+        # but that method delegates to CF1_7Check.check_standard_name(), which loops through
+        # ALL the variables; this takes the absolute core of that check and ASSUMES that the
+        # current variable being checked is a coordinate variable, auxiliary coordinate
+        # variable, axis variable, flag variable, or geophysical variable
+        std_name = getattr(var, "standard_name", False)
+        valid_std_name = std_name in self.cf1_7._std_names
+
+        # should have compliant units
+        # NOTE: units are checked extensively in self.check_units(), which delegates
+        # to CF1_7Check.check_units() --> CF1_6Check.check_units(), which loops through
+        # ALL variables; this takes the absolute core and assumes that the variable does
+        # not need dimensionless units nor are the units to be compared with any known
+        # deprecated ones; it would be nice to reuse machinery, but the similarly convoluted
+        # CF1_6Check.check_units() method is too tangled to use directly and would cause a huge
+        # time increase
+        units = getattr(var, "units", None)
+        has_udunits = (units is not None and cf_util.units_known(units))
+
+        return (avar_val and valid_std_name and has_udunits)
+
+    def check_gts_ingest_requirements(self, ds):
         """
-        Check if a dataset has a global gts_ingest attribute and if any
-        variables also have the gts_ingest attribute.
+        If a dataset is flagged for gts_ingest, check which variables
+        qualify for ingest.
 
         According to https://ioos.github.io/ioos-metadata/ioos-metadata-profile-v1-2.html#requirements-for-ioos-dataset-gts-ingest,
-        the gts_ingest is "required, if applicable". Because the Compliance Checker
-        cannot accurately guess applicability of a certain dataset or variable,
-        this check simply verifies that if it exists, the value is a string
-        denoting "true" or "false".
-
-        Any variables which a user would like ingested must also contain the
-        gts_ingest attribute with a value of true. The variable must:
+        the gts_ingest is "required, if applicable". Any variables which a user
+        would like ingested must also contain the gts_ingest attribute with a
+        value of true. The variable must:
           - have a valid CF standard_name attribute (already checked)
           - have an ancillary variable reqpresenting QARTOD aggregate flags
           - have a valid udunits units attribute (already checked)
 
-        Args:
-            ds (netCDF4.Dataset): open Dataset
+        Parameters
+        ----------
+        ds (netCDF4.Dataset): open Dataset
 
-        Returns:
-            list of Result objects
+        Returns
+        -------
+        Result
         """
 
-        default_pass_result = Result(BaseCheck.HIGH, True, "gts_ingest", ["gts_ingest"])
-
-        results = []
-
-        # check global
-        glb_msg = ("If provided, the global attribute \"gts_ingest\" must be a "
-                   "string and its value must be one of \"true\" or \"false\"")
+        # is dataset properly flagged for ingest?
         glb_gts_attr = getattr(ds, "gts_ingest", None)
         if glb_gts_attr and (glb_gts_attr=="true" or glb_gts_attr=="false"):
             do_gts = True if glb_gts_attr == "true" else False
-            results.append(Result(BaseCheck.HIGH, True, "gts_ingest", [glb_msg]))
         else:
             do_gts = False
-            results.append(default_pass_result)
 
         # check variables
-        var_msg = ("The attribute \"gts_ingest\" of variable \"{v.name}\" "
-                   " must fulfill the following:\n"
-                   "  - must be a string with value \"true\" or \"false\";\n"
-                   "  - have a valid CF standard_name attribute (already checked);\n"
-                   "  - have an ancillary variable representing QARTOD aggregate flags;\n"
-                   "  - have a valid udunits units attribute\n"
-                   "The global attribute \"gts_ingest\" "
-                   "must also have a value of \"true\".")
-
-        for v in ds.get_variables_by_attributes(gts_ingest=lambda x: x=="true"):
-            results.append(self._check_var_gts_ingest(ds, v, do_gts, var_msg.format(v=v)))
+        all_passed_ingest_reqs = True # default
 
-        return results
+        if do_gts: # execute if dataset flagged for ingest
+
+            var_passed_ingest_reqs = set()
+            var_passed_ingest_msg  = "The following variables qualifed for GTS Ingest:"
+            var_failed_ingest_msg  = "The following variables did not qualify for GTS Ingest:"
+
+            for v in ds.get_variables_by_attributes(gts_ingest=lambda x: x=="true"):
+                var_passed_ingest_reqs.add((v.name, self._var_qualifies_for_gts_ingest(ds, v)))
+
+            for var_name, pass_result in var_passed_ingest_reqs:
+                if pass_result:
+                    var_passed_ingest_msg += "\n - {}".format(var_name)
+                else:
+                    var_failed_ingest_msg += "\n - {}".format(var_name)
+                    all_passed_ingest_reqs = False
+    
+            # join messages together
+            var_passed_ingest_msg += "\n{}".format(var_failed_ingest_msg)
+
+        else:
+            var_passed_ingest_msg = "Dataset not flagged for GTS Ingest; skipping variable checks"
+
+        return Result(
+            BaseCheck.HIGH,
+            all_passed_ingest_reqs,
+            "gts_ingest requirements",
+            [var_passed_ingest_msg]
+        )
 
     def check_instrument_variables(self, ds):
         """
diff --git a/requirements.txt b/requirements.txt
index b84e7ae..6e5f3e1 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,3 +1,4 @@
+cftime>=1.1.0
 OWSLib>=0.8.3
 lxml>=3.2.1
 cf-units>=2


Test patch:
diff --git a/compliance_checker/tests/test_ioos_profile.py b/compliance_checker/tests/test_ioos_profile.py
index c7daf3c..412da6a 100644
--- a/compliance_checker/tests/test_ioos_profile.py
+++ b/compliance_checker/tests/test_ioos_profile.py
@@ -1,5 +1,6 @@
 from compliance_checker.ioos import (IOOS0_1Check, IOOS1_1Check, IOOS1_2Check,
-                                     NamingAuthorityValidator)
+                                     NamingAuthorityValidator,
+                                     IOOS1_2_PlatformIDValidator)
 from compliance_checker.tests.resources import STATIC_FILES
 from compliance_checker.tests import BaseTestCase
 from compliance_checker.tests.helpers import MockTimeSeries, MockVariable
@@ -321,7 +322,7 @@ class TestIOOS1_2(BaseTestCase):
         temp = ds.createVariable("temp", np.float64, fill_value=9999999999.) # _FillValue
         temp.setncattr("missing_value", 9999999999.)
         temp.setncattr("standard_name", "sea_surface_temperature")
-        temp.setncattr("standard_name_uri", "http://cfconventions.org/Data/cf-standard-names/64/build/cf-standard-name-table.html")
+        temp.setncattr("standard_name_url", "http://cfconventions.org/Data/cf-standard-names/64/build/cf-standard-name-table.html")
         temp.setncattr("units", "degree_C")
         temp.setncattr("platform", "myPlatform")
 
@@ -344,7 +345,7 @@ class TestIOOS1_2(BaseTestCase):
         ds = MockTimeSeries(default_fill_value=9999999999.) # time, lat, lon, depth 
 
         ds.variables["time"].setncattr("standard_name", "time")
-        ds.variables["time"].setncattr("standard_name_uri", "time")
+        ds.variables["time"].setncattr("standard_name_url", "http://cfconventions.org/Data/cf-standard-names/64/build/cf-standard-name-table.html")
         ds.variables["time"].setncattr("units", "hours since 1970-01-01T00:00:00")
         ds.variables["time"].setncattr("missing_value", 9999999999.)
 
@@ -419,40 +420,68 @@ class TestIOOS1_2(BaseTestCase):
         result_list = self.ioos.check_creator_and_publisher_type(ds)
         self.assertTrue(all(res.value for res in result_list))
 
-    def test_check_gts_ingest(self):
+    def test_check_gts_ingest_global(self):
         ds = MockTimeSeries() # time, lat, lon, depth
 
-        # no gts_ingest, should pass
-        results = self.ioos.check_gts_ingest(ds)
-        scored, out_of, messages = get_results(results)
-        self.assertEqual(scored, out_of)
+        # no gts_ingest_requirements, should pass
+        result = self.ioos.check_gts_ingest_global(ds)
+        self.assertTrue(result.value)
 
-        # global
+        # passing value
         ds.setncattr("gts_ingest", "true")
-        results = self.ioos.check_gts_ingest(ds)
-        scored, out_of, messages = get_results(results)
-        self.assertEqual(scored, out_of)
+        result = self.ioos.check_gts_ingest_global(ds)
+        self.assertTrue(result.value)
+
+        ds.setncattr("gts_ingest", "false")
+        result = self.ioos.check_gts_ingest_global(ds)
+        self.assertTrue(result.value)
+
+        ds.setncattr("gts_ingest", "notgood")
+        result = self.ioos.check_gts_ingest_global(ds)
+        self.assertFalse(result.value)
+
+    def test_check_gts_ingest_requirements(self):
+        ds = MockTimeSeries() # time, lat, lon, depth
+
+        # no gts_ingest_requirements, should pass
+        result = self.ioos.check_gts_ingest_requirements(ds)
+        self.assertTrue(result.value)
+
+        # flag for ingest, no variables flagged - default pass
+        ds.setncattr("gts_ingest", "true")
+        result = self.ioos.check_gts_ingest_requirements(ds)
+        self.assertTrue(result.value)
 
         # give one variable the gts_ingest attribute
+        # no standard_name or ancillary vars, should fail
+        ds.variables["time"].setncattr("gts_ingest", "true")
+        result = self.ioos.check_gts_ingest_requirements(ds)
+        self.assertFalse(result.value)
+
         # no ancillary vars, should fail
         ds.variables["time"].setncattr("gts_ingest", "true")
-        results = self.ioos.check_gts_ingest(ds)
-        scored, out_of, messages = get_results(results)
-        self.assertLess(scored, out_of)
+        ds.variables["time"].setncattr("standard_name", "time")
+        result = self.ioos.check_gts_ingest_requirements(ds)
+        self.assertFalse(result.value)
 
         # set ancillary var with bad standard name
         tmp = ds.createVariable("tmp", np.byte, ("time",))
         tmp.setncattr("standard_name", "bad")
         ds.variables["time"].setncattr("ancillary_variables", "tmp")
-        results = self.ioos.check_gts_ingest(ds)
-        scored, out_of, messages = get_results(results)
-        self.assertLess(scored, out_of)
+        result = self.ioos.check_gts_ingest_requirements(ds)
+        self.assertFalse(result.value)
 
-         # good ancillary var standard name
+        # good ancillary var standard name, time units are bad
         tmp.setncattr("standard_name", "aggregate_quality_flag")
-        results = self.ioos.check_gts_ingest(ds)
-        scored, out_of, messages = get_results(results)
-        self.assertEqual(scored, out_of)
+        ds.variables["time"].setncattr("units", "bad since bad")
+        result = self.ioos.check_gts_ingest_requirements(ds)
+        self.assertFalse(result.value)
+
+        # good ancillary var stdname, good units, pass
+        tmp.setncattr("standard_name", "aggregate_quality_flag")
+        ds.variables["time"].setncattr("units", "seconds since 1970-01-01T00:00:00Z")
+        result = self.ioos.check_gts_ingest_requirements(ds)
+        self.assertTrue(result.value)
 
     def test_check_instrument_variables(self):
 
@@ -570,6 +599,24 @@ class TestIOOS1_2(BaseTestCase):
                          "naming_authority should either be a URL or a "
                          "reversed DNS name (e.g \"edu.ucar.unidata\")")
 
+    def test_platform_id_validation(self):
+        attn = "platform_id"
+        attv = "alphaNum3R1C"
+        v = IOOS1_2_PlatformIDValidator()
+        self.assertTrue(v.validate(attn, attv)[0])
+        
+        attv = "alpha"
+        v = IOOS1_2_PlatformIDValidator()
+        self.assertTrue(v.validate(attn, attv)[0])
+
+        attv = "311123331112"
+        v = IOOS1_2_PlatformIDValidator()
+        self.assertTrue(v.validate(attn, attv)[0])
+
+        attv = "---fail---"
+        v = IOOS1_2_PlatformIDValidator()
+        self.assertFalse(v.validate(attn, attv)[0])
+
     def test_check_platform_cf_role(self):
         """
         Check that cf_role inside platform variables only allows certain