Repository: ioos/compliance-checker
Pull request #770 (2020-03-16 20:02:48)

WIP Separate the gts_ingest global and variable checks
WIP to address #757 and #759

daltonkell: Pinging @benjwadams on this review. I tried to reuse some existing machinery for checking standard names and units of variables, but extracting it out of the CF module is a lift I'm not ready to undertake just yet (way outside the scope of this pull request or the issues it addresses). I'd like to know your opinion on the effectiveness of my choices:

https://github.com/ioos/compliance-checker/blob/e699f62e0d94644881dffffacbaab22371e951e4/compliance_checker/ioos.py#L967-L985

daltonkell: @mwengren I started working on #759 as it was basically in conjunction with #757. This is not in the milestone officially, but since #757 is, it wouldn't have made sense to not work on it. 

With these changes, testing against https://pae-paha.pacioos.hawaii.edu/erddap/tabledap/WQB-04.html (flagged for ingest) yields the following results:

```
...

gts_ingest requirements
* The following variables qualifed for GTS Ingest:
The following variables did not qualify for GTS Ingest:
 - oxygen_saturation
 - oxygen
 - turbidity
 - temperature
 - chlorophyll
 - salinity

...
```

These variables fail because they do not have an ancillary variable representing the QARTOD aggregate flag. What do you think so far?
daltonkell: Whoops -- I can't demote this to a draft PR, but it's still a draft PR. __NOT READY FOR MERGING!__
mwengren: @daltonkell Great, glad you are working on #759!  That will be a huge addition.

I see an issue here.  Looking only at the `temperature` variable on [WQB-04](https://pae-paha.pacioos.hawaii.edu/erddap/tabledap/WQB-04.das), I think it meets the requirements.  Checker should find the `ancillary_variables=temperature_qc_agg` (alongside many others), and see that is has a valid `standard_name=aggreagate_quality_flag`.  

Also, how is it determined which variables end up in the list of GTS-qualified|not qualified variables?  This is something I hadn't thought about.  With that test dataset, obviously there are many variables that are QC-related and not meant for publication on the GTS (they may be meant to be harvested by NDBC in connection to a data variable they describe, but they don't themselves qualify for GTS publication because they shouldn't have ancillary variables, etc).  

Seems the checker should output a list all variables that don't meet GTS requirements, even if they're QC-related.  So this should be a long list with all the QC-related variables tagged for not meeting GTS requirements.  Hopefully that's what happens here, can't quite tell.

daltonkell: Relating to the issue you are seeing -- very strange. I downloaded both the `.nc` and `.ncCF` files and `ncdump` -ed them, finding no `standard_name` of `aggregate_quality_flag`, but there it is in the `DAS`.

Which format would you typically use for testing?
daltonkell: > Also, how is it determined which variables end up in the list of GTS-qualified|not qualified variables?

I can use "meets requirements" instead of "qualified" if that's clearer. All this is doing is showing which variables met the requirements or not; I thought "qualified" would be more concise, but if it's actually referring to another categorization process, I'll change it to avoid any confusion.
mwengren: > I can use "meets requirements" instead of "qualified" if that's clearer. All this is doing is showing which variables met the requirements or not; I thought "qualified" would be more concise, but if it's actually referring to another categorization process, I'll change it to avoid any confusion.

Either wording is fine I think.  My question was more about how many variables in this particular dataset end up in the not qualified|not meeting requirements list.  Do all the QC-related variables end up being listed there?

> Relating to the issue you are seeing -- very strange. I downloaded both the .nc and .ncCF files and ncdump -ed them, finding no standard_name of aggregate_quality_flag, but there it is in the DAS.

> Which format would you typically use for testing?

Here's the command I ran for my own test of that dataset:
```
compliance-checker -t ioos -f html https://pae-paha.pacioos.hawaii.edu/erddap/tabledap/WQB-04 > WQB-04-ioos.html
```
I think your question gets back to discussion on #751.  I'd say `.ncCF` or `.ncCFMA` would be the right ones to pull out of ERDDAP.  

For WQB-04, the [.ncCF headers response](https://pae-paha.pacioos.hawaii.edu/erddap/tabledap/WQB-04.ncCFHeader?&time%3E=2020-01-06T16:00:00Z) contain those attributes, so files should as well.  

I'm not sure what CC is actually requesting under the hood, however!





daltonkell: Ahh ok I think I found the problem -- I downloaded the `.ncCF` file earlier, but it didn't request all the data! I re-downloaded it and here are the new results:

```
Running Compliance Checker on the datasets from: ['WQB-04.ncCF']
ut_scale(): NULL factor argument

--------------------------------------------------------------------------------
                         IOOS Compliance Checker Report                         
                                    ioos:1.2                                    
      https://ioos.github.io/ioos-metadata/ioos-metadata-profile-v1-2.html      
--------------------------------------------------------------------------------
                               Corrective Actions                               
WQB-04.ncCF has 28 potential issues


                               Highly Recommended                               
--------------------------------------------------------------------------------
cf_role
* attribute cf_role in variable platform1 not present

creator_country
* creator_country not present

creator_sector
* creator_sector not present

gts_ingest requirements
* The following variables qualifed for GTS Ingest:
 - turbidity
 - temperature
 - salinity
 - chlorophyll
 - oxygen
 - oxygen_saturation
The following variables did not qualify for GTS Ingest:
 - salinity_qc_din
 - turbidity_qc_spk
 - chlorophyll_qc_syn
 - chlorophyll_qc_nbr
 - oxygen_qc_gap
 - temperature_qc_loc
 - chlorophyll_qc_clm
 - oxygen_saturation_qc_agg
 - oxygen_saturation_qc_syn
 - salinity_qc_mvr
 - salinity_qc_syn
 - chlorophyll_qc_mvr
 - chlorophyll_qc_rtc
 - turbidity_qc_clm
 - oxygen_qc_rtc
 - oxygen_saturation_qc_rng
 - temperature_qc_rtc
 - temperature_qc_syn
 - temperature_qc_nbr
 - salinity_qc_agg
 - temperature_qc_rng
 - temperature_qc_flt
 - temperature_qc_din
 - salinity_qc_nbr
 - chlorophyll_qc_agg
 - oxygen_qc_syn
 - turbidity_qc_atn
 - oxygen_saturation_qc_loc
 - oxygen_saturation_qc_rtc
 - temperature_qc_crv
 - oxygen_qc_loc
 - chlorophyll_qc_atn
 - temperature_qc_agg
 - salinity_qc_atn
 - oxygen_qc_clm
 - turbidity_qc_syn
 - turbidity_qc_nbr
 - salinity_qc_loc
 - salinity_qc_rng
 - turbidity_qc_gap
 - oxygen_qc_rng
 - oxygen_saturation_qc_clm
 - oxygen_saturation_qc_atn
 - turbidity_qc_agg
 - temperature_qc_spk
 - temperature_qc_gap
 - turbidity_qc_loc
 - turbidity_qc_rng
 - temperature_qc_clm
 - chlorophyll_qc_gap
 - oxygen_qc_spk
 - chlorophyll_qc_rng
 - salinity_qc_clm
 - oxygen_qc_flt
 - oxygen_saturation_qc_gap
 - salinity_qc_rtc
 - oxygen_qc_agg
 - temperature_qc_mvr
 - salinity_qc_gap
 - oxygen_qc_atn
 - oxygen_saturation_qc_spk
 - salinity_qc_spk
 - oxygen_qc_mvr
 - oxygen_saturation_qc_flt
 - chlorophyll_qc_spk
 - chlorophyll_qc_flt
 - chlorophyll_qc_loc
 - turbidity_qc_mvr
 - oxygen_qc_nbr
 - turbidity_qc_flt
 - oxygen_saturation_qc_nbr
 - turbidity_qc_rtc
 - temperature_qc_atn
 - salinity_qc_flt
 - oxygen_saturation_qc_mvr
 - salinity_qc_crv

platform
* The global attribute "platform" must be a single string containing no blank characters; it is In Situ Ocean-based Platforms > Buoys, In Situ Ocean-based Platforms > Moorings

...
```

Obviously that first part about a `NULL factor argument` will be investigated, but I think this answers your question "will all the QC variables be included?"

Patch:
diff --git a/compliance_checker/ioos.py b/compliance_checker/ioos.py
index 5e94ed1..44bf2b8 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
@@ -412,7 +413,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 +452,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,7 +463,6 @@ class IOOS1_2Check(IOOSNCCheck):
             #'creator_type',
             'institution',
             'instrument',
-            'ioos_ingest',
             'keywords',
             'platform_id',
             'publisher_address',
@@ -888,89 +886,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):
         """
-        Helper function for check_gts_ingest(). Check that a given variable
+        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):
+        """
+        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):
         """


Test patch:
diff --git a/compliance_checker/tests/test_ioos_profile.py b/compliance_checker/tests/test_ioos_profile.py
index c7daf3c..99ec6f9 100644
--- a/compliance_checker/tests/test_ioos_profile.py
+++ b/compliance_checker/tests/test_ioos_profile.py
@@ -321,7 +321,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 +344,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 +419,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):