Repository: ioos/compliance-checker
Pull request #694 (2019-11-12 22:07:19)

Check Appendix A attributes for CF 1.6 and 1.7
Adds a check for CF 1.6 and 1.7 attributes, as well as a general
framework to execute these checks.

Practically the same as #690, recreating to see if AppVeyor failures can  be prevented.

benjwadams: @ocefpaf, do you know why the AppVeyor might be failing?  All tests pass when run under Python 2.7 and 3 locally.  The only thing I can think of is that AppVeyor is somehow caching some of the test netCDF files.
ocefpaf: The failing test is:

```
================================== FAILURES ===================================
___________________ TestCLI.test_multi_checker_return_value ___________________
self = test_multi_checker_return_value (compliance_checker.tests.test_cli.TestCLI)
    def test_multi_checker_return_value(self):
        '''
        Tests that any failure for multiple checkers results in a failure return
        status
        '''
        # CF should pass here
        return_value, errors = ComplianceChecker.run_checker(
            ds_loc=STATIC_FILES['ncei_gold_point_1'],
            verbose=0,
            criteria='strict',
            checker_names=['cf:1.6'],
            output_filename=self.path,
            output_format='text'
        )
>       self.assertTrue(return_value)
E       AssertionError: False is not true
compliance_checker\tests\test_cli.py:227: AssertionError
```

We would need to check what return value we are getting there. Windows is not like *nix where we get 0 or 1. There are multiple passing and failing values.
benjwadams: The failing test should be returning a Python boolean 
via `(all(cs.passtree(groups, limit) ...)` so should not be affected by different return status conventions, AFAICT.
benjwadams: @ocefpaf, @Bobfrat tried out installing this branch on Windows under Conda.  We weren't able to reproduce test failures encountered in CircleCI when installing compliance-checker via Conda.  I'm going to merge this into the `cf_1_7_dev_rebase` branch and we can pursue the root cause of the test failures further prior to merging into `master`.

Patch:
diff --git a/compliance_checker/cf/cf.py b/compliance_checker/cf/cf.py
index d8c5907..68b9eca 100644
--- a/compliance_checker/cf/cf.py
+++ b/compliance_checker/cf/cf.py
@@ -138,6 +138,13 @@ class CFBaseCheck(BaseCheck):
         self._find_metadata_vars(ds)
         self._find_cf_standard_name_table(ds)
         self._find_geophysical_vars(ds)
+        coord_containing_vars = ds.get_variables_by_attributes(coordinates=
+                                       lambda val: isinstance(val, basestring))
+        self.coord_data_vars = {coord_var_name for var in
+                                coord_containing_vars for
+                                coord_var_name in
+                                var.coordinates.strip().split(' ') if
+                                coord_var_name in ds.variables}
 
     def check_conventions_version(self, ds):
         '''
@@ -677,6 +684,142 @@ class CFBaseCheck(BaseCheck):
         else:
             return standard_name, None
 
+    def check_appendix_a(self, ds):
+        """
+        Validates a CF dataset against the contents of its Appendix A table for
+        attribute types and locations. Returns a list of results with the
+        outcomes of the Appendix A validation results against the existing
+        attributes in the docstring.
+
+        :param netCDF4.Variable var: a variable in an existing NetCDF dataset
+        :param netCDF4.Dataset ds: An open netCDF dataset
+        :rtype: list
+        :return: A list of results corresponding to the results returned
+        """
+        possible_global_atts = (set(ds.ncattrs()).
+                                intersection(self.appendix_a.keys()))
+        results = []
+        for global_att_name in possible_global_atts:
+            global_att = ds.getncattr(global_att_name)
+            att_dict = self.appendix_a[global_att_name]
+            att_loc = att_dict['attr_loc']
+            if att_dict['cf_section'] is not None:
+                subsection_test = '.'.join(att_dict['cf_section'].split('.')
+                                        [:2])
+
+                section_loc = self.section_titles.get(subsection_test,
+                                        att_dict['cf_section'])
+            else:
+                section_loc = None
+            test_ctx = TestCtx(BaseCheck.HIGH, section_loc)
+
+            test_ctx.out_of += 1
+            if 'G' not in att_loc:
+                test_ctx.messages.append("Attribute {} should not be in global "
+                                         " attributes. Valid location(s) are "
+                                         "[{}]".format(global_att,
+                                                        ', '.join(att_loc)))
+            else:
+                result = self._handle_dtype_check(global_att, global_att_name,
+                                                  att_dict)
+                if not result[0]:
+                    test_ctx.messages.append(result[1])
+                else:
+                    test_ctx.score += 1
+            results.append(test_ctx.to_result())
+
+        noncoord_vars = set(ds.variables) - set(self.coord_data_vars)
+        for var_set, coord_letter, var_type, in (
+                                        (self.coord_data_vars, 'C',
+                                         'coordinate'),
+                                        (noncoord_vars, 'D', 'non-coordinate')):
+            for var_name in var_set:
+                var = ds.variables[var_name]
+                possible_attrs = (set(var.ncattrs()).
+                                  intersection(self.appendix_a.keys()))
+                for att_name in possible_attrs:
+                    att_dict = self.appendix_a[att_name]
+                    if att_dict['cf_section'] is not None:
+                        subsection_test = '.'.join(att_dict['cf_section'].split('.')
+                                                [:2])
+
+                        section_loc = self.section_titles.get(subsection_test,
+                                                att_dict['cf_section'])
+                    else:
+                        section_loc = None
+                    test_ctx = TestCtx(BaseCheck.HIGH, section_loc,
+                                       variable=var_name)
+                    att_loc = att_dict['attr_loc']
+                    att = var.getncattr(att_name)
+                    test_ctx.out_of += 1
+                    if coord_letter not in att_loc:
+                        test_ctx.messages.append("Attribute {} should not be in variable {} "
+                                                "attributes for variable {}. Valid location(s) are "
+                                                "[{}]".format(att_name,
+                                                              var_type,
+                                                              var_name,
+                                                              ', '.join(att_loc)
+                                                              ))
+                    else:
+                        result = self._handle_dtype_check(att, att_name,
+                                                          att_dict, var)
+                        if not result[0]:
+                            test_ctx.messages.append(result[1])
+                        else:
+                            test_ctx.score += 1
+                    results.append(test_ctx.to_result())
+
+        return results
+
+    def _handle_dtype_check(self, attribute, attr_name, attr_dict,
+                            variable=None):
+        """
+        Helper function for Appendix A checks.
+
+        :param attribute: The value of the attribute being checked
+        :param str attr_name: The name of the attribute being processed
+        :param dict attr_dict: The dict entry with type and attribute location
+                               information corresponding to this attribute
+        :rtype: tuple
+        :return: A two-tuple that contains pass/fail status as a boolean and
+                 a message string (or None if unset) as the second element.
+        """
+        attr_type = attr_dict['Type']
+        if variable is None and 'G' not in attr_dict['attr_loc']:
+            raise ValueError('Non-global attributes must be associated with a '
+                             ' variable')
+        attr_str = ('Global attribute {}'.format(attr_name)
+                    if 'G' in attr_dict['attr_loc'] and variable is None
+                    else "Attribute {} in variable {}".format(attr_name,
+                                                              variable.name))
+        if attr_type == 'S':
+            if not isinstance(attribute, basestring):
+                return (False,
+                        "{} must be a string".format(attr_str))
+        else:
+            # if it's not a string, it should have a numpy dtype
+            underlying_dtype = getattr(attribute, 'dtype', None)
+            if underlying_dtype is None:
+                return (False,
+                        "{} must be a numeric type".format(attr_str))
+            # both D and N should be some kind of numeric value
+            is_numeric = np.issubdtype(underlying_dtype, np.number)
+            if attr_type == 'N':
+                if not is_numeric:
+                    return (False,
+                            "{} must be numeric".format(attr_str))
+            elif attr_type == 'D':
+                # TODO: handle edge case where variable is unset here
+                var_dtype = getattr(variable, 'dtype', None)
+                if not is_numeric or (underlying_dtype != var_dtype):
+                    return (False,
+                            "{} must be numeric and must match".format(attr_str))
+            else:
+                # If we reached here, we fell off with an unrecognized type
+                return("{} has unrecognized type '{}'".format(attr_str,
+                                                              attr_type))
+        # pass if all other possible failure conditions have been evaluated
+        return (True, None)
 
 class CFNCCheck(BaseNCCheck, CFBaseCheck):
     """Inherits from both BaseNCCheck and CFBaseCheck to support
@@ -685,6 +828,52 @@ class CFNCCheck(BaseNCCheck, CFBaseCheck):
     CFNCCheck."""
     pass
 
+appendix_a_base = {'Conventions': {'Type': 'S', 'attr_loc': {'G'}, 'cf_section': None},
+                  '_FillValue': {'Type': 'D', 'attr_loc': {'D', 'C'}, 'cf_section': None},
+                  'add_offset': {'Type': 'N', 'attr_loc': {'D'}, 'cf_section': '8.1'},
+                  'ancillary_variables': {'Type': 'S', 'attr_loc': {'D'}, 'cf_section': '3.4'},
+                  'axis': {'Type': 'S', 'attr_loc': {'C'}, 'cf_section': '4'},
+                  'bounds': {'Type': 'S', 'attr_loc': {'C'}, 'cf_section': '7.1'},
+                  'calendar': {'Type': 'S', 'attr_loc': {'C'}, 'cf_section': '4.4.1'},
+                  'cell_measures': {'Type': 'S', 'attr_loc': {'D'}, 'cf_section': '7.2'},
+                  'cell_methods': {'Type': 'S', 'attr_loc': {'D'}, 'cf_section': '7.3'},
+                  # cf_role type is "C" in document, which does not correspond
+                  # to types used, replaced with "S"
+                  'cf_role': {'Type': 'S', 'attr_loc': {'C'}, 'cf_section': '9.5'},
+                  'climatology': {'Type': 'S', 'attr_loc': {'C'}, 'cf_section': '7.4'},
+                  # comment was removed in this implementation
+                  'compress': {'Type': 'S', 'attr_loc': {'C'}, 'cf_section': '8.2'},
+                  'coordinates': {'Type': 'S', 'attr_loc': {'D'}, 'cf_section': '5'},
+                  # featureType type is "C" in document, which does not
+                  # correspond to types used, replaced with "S"
+                  'featureType': {'Type': 'S', 'attr_loc': {'G'}, 'cf_section': '9.4'},
+                  'flag_masks': {'Type': 'D', 'attr_loc': {'D'}, 'cf_section': '3.5'},
+                  'flag_meanings': {'Type': 'S', 'attr_loc': {'D'}, 'cf_section': '3.5'},
+                  'flag_values': {'Type': 'D', 'attr_loc': {'D'}, 'cf_section': '3.5'},
+                  'formula_terms': {'Type': 'S', 'attr_loc': {'C'}, 'cf_section': '4.3.2'},
+                  'grid_mapping': {'Type': 'S', 'attr_loc': {'D'}, 'cf_section': '5.6'},
+                  'history': {'Type': 'S', 'attr_loc': {'G'}, 'cf_section': None},
+                  #'instance_dimension': {'Type': 'N', 'attr_loc': {'D'}, 'cf_section': '9.3'},
+                  'institution': {'Type': 'S', 'attr_loc': {'G', 'D'}, 'cf_section': '2.6.2'},
+                  'leap_month': {'Type': 'N', 'attr_loc': {'C'}, 'cf_section': '4.4.1'},
+                  'leap_year': {'Type': 'N', 'attr_loc': {'C'}, 'cf_section': '4.4.1'},
+                  'long_name': {'Type': 'S', 'attr_loc': {'D', 'C'}, 'cf_section': '3.2'},
+                  'missing_value': {'Type': 'D', 'attr_loc': {'D', 'C'}, 'cf_section': '2.5.1'},
+                  'month_lengths': {'Type': 'N', 'attr_loc': {'C'}, 'cf_section': '4.4.1'},
+                  'positive': {'Type': 'S', 'attr_loc': {'C'}, 'cf_section': None},
+                  'references': {'Type': 'S', 'attr_loc': {'G', 'D'}, 'cf_section': '2.6.2'},
+                  #'sample_dimension': {'Type': 'N', 'attr_loc': {'D'}, 'cf_section': '9.3'},
+                  'scale_factor': {'Type': 'N', 'attr_loc': {'D'}, 'cf_section': '8.1'},
+                  'source': {'Type': 'S', 'attr_loc': {'G', 'D'}, 'cf_section': '2.6.2'},
+                  'standard_error_multiplier': {'Type': 'N',
+                                                'attr_loc': {'D'},
+                                                'cf_section': None},
+                  'standard_name': {'Type': 'S', 'attr_loc': {'D', 'C'}, 'cf_section': '3.3'},
+                  'title': {'Type': 'S', 'attr_loc': {'G'}, 'cf_section': None},
+                  'units': {'Type': 'S', 'attr_loc': {'D', 'C'}, 'cf_section': '3.1'},
+                  'valid_max': {'Type': 'N', 'attr_loc': {'D', 'C'}, 'cf_section': None},
+                  'valid_min': {'Type': 'N', 'attr_loc': {'D', 'C'}, 'cf_section': None},
+                  'valid_range': {'Type': 'N', 'attr_loc': {'D', 'C'}, 'cf_section': None}}
 
 class CF1_6Check(CFNCCheck):
     """CF-1.6-specific implementation of CFBaseCheck; supports checking
@@ -703,6 +892,7 @@ class CF1_6Check(CFNCCheck):
         2: 'Warnings',
         1: 'Info'
     }
+    appendix_a = appendix_a_base
 
     def __init__(self): # initialize with parent methods and data
         super(CF1_6Check, self).__init__()
@@ -2953,7 +3143,6 @@ class CF1_6Check(CFNCCheck):
                 valid_info.out_of += 1
                 valid_info.messages.append('§7.3.3 Invalid cell_methods keyword "{}" for variable {}. Must be one of [interval, comment]'.format(keyword, var.name))
 
-
         # Ensure concatenated reconstructed matches are the same as the
         # original string.  If they're not, there's likely a formatting error
         valid_info.assert_true(''.join(m.group(0)
@@ -3294,7 +3483,8 @@ class CF1_6Check(CFNCCheck):
         """
         # Due to case insensitive requirement, we list the possible featuretypes
         # in lower case and check using the .lower() method
-        feature_list = ['point', 'timeseries', 'trajectory', 'profile', 'timeseriesprofile', 'trajectoryprofile']
+        feature_list = ['point', 'timeseries', 'trajectory', 'profile',
+                        'timeseriesprofile', 'trajectoryprofile']
 
         feature_type = getattr(ds, 'featureType', None)
         valid_feature_type = TestCtx(BaseCheck.HIGH, '§9.1 Dataset contains a valid featureType')
@@ -3433,6 +3623,15 @@ class CF1_7Check(CF1_6Check):
     _cc_spec_version    = '1.7'
     _cc_url             = 'http://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/cf-conventions.html'
 
+    appendix_a = appendix_a_base.copy()
+    appendix_a.update({
+        'actual_range': {'Type': 'N', 'attr_loc': {'D', 'C'}, 'cf_section': '2.5.1'},
+        'comment': {'Type': 'S', 'attr_loc': {'G', 'D', 'C'}, 'cf_section': '2.6.2'},
+        'external_variables': {'Type': 'S', 'attr_loc': {'G'}, 'cf_section': '2.6.3'},
+        'actual_range': {'Type': 'N', 'attr_loc': {'D', 'C'}, 'cf_section': '2.5.1'},
+        'scale_factor': {'Type': 'N', 'attr_loc': {'D', 'C'}, 'cf_section': '8.1'}
+    })
+
     def __init__(self):
         super(CF1_7Check, self).__init__()
 
@@ -3846,17 +4045,17 @@ class CF1_7Check(CF1_6Check):
             deprecated_units,
             self._check_dimensionless_vertical_coordinate_1_6,
             dimless_vertical_coordinates_1_7)
-        ) 
+        )
 
         ret_val.extend(self._check_dimensionless_vertical_coordinates(
             ds,
             deprecated_units,
             self._check_dimensionless_vertical_coordinate_1_7,
             dimless_vertical_coordinates_1_7)
-        ) 
+        )
 
         return ret_val
-     
+
 
 class CFNCCheck(BaseNCCheck, CFBaseCheck):
 
diff --git a/compliance_checker/suite.py b/compliance_checker/suite.py
index b7af8fa..1f35da8 100644
--- a/compliance_checker/suite.py
+++ b/compliance_checker/suite.py
@@ -14,6 +14,7 @@ from netCDF4 import Dataset
 from lxml import etree as ET
 from distutils.version import StrictVersion
 from compliance_checker.base import fix_return_value, Result, GenericFile
+from compliance_checker.cf.cf import CFBaseCheck
 from owslib.sos import SensorObservationService
 from owslib.swe.sensor.sml import SensorML
 from compliance_checker.protocols import opendap, netcdf, cdl
@@ -276,7 +277,7 @@ class CheckSuite(object):
         args = [(name, self.checkers[name]) for name in checker_names if name in self.checkers]
         valid = []
 
-        all_checked = set([a[1] for a in args])  # only class types
+        all_checked = set(a[1] for a in args)  # only class types
         checker_queue = set(args)
         while len(checker_queue):
             name, a = checker_queue.pop()
@@ -750,7 +751,6 @@ class CheckSuite(object):
         @param list raw_scores: list of raw scores (Result objects)
         """
 
-        # BEGIN INTERNAL FUNCS ########################################
         def trim_groups(r):
             if isinstance(r.name, tuple) or isinstance(r.name, list):
                 new_name = r.name[1:]


Test patch:
diff --git a/compliance_checker/tests/test_cf.py b/compliance_checker/tests/test_cf.py
index 9a3143f..e8ddbe3 100644
--- a/compliance_checker/tests/test_cf.py
+++ b/compliance_checker/tests/test_cf.py
@@ -131,7 +131,8 @@ class TestCF1_6(BaseTestCase):
         # delete the dataset and start over to create the variable with _FillValue at time of creation
         del ds
         ds = MockTimeSeries()
-        ds.createVariable("temp", np.float64, dimensions=("time"), fill_value=np.float(99999999999999999999.))
+        ds.createVariable("temp", np.float64, dimensions=("time"),
+                          fill_value=np.float(99999999999999999999.))
 
         # give temp _FillValue as a float, expect good result
         result = self.cf.check_child_attr_data_types(ds)
@@ -154,6 +155,17 @@ class TestCF1_6(BaseTestCase):
 
         # TODO for CF-1.7: actual_range, actual_min/max
 
+    def test_appendix_a(self):
+        dataset = self.load_dataset(STATIC_FILES['bad_data_type'])
+        self.cf.setup(dataset)
+        aa_results = self.cf.check_appendix_a(dataset)
+        # institution is in salinity, this shouldn't be present
+        flat_messages = {msg for res in aa_results for msg in res.msgs}
+        self.assertIn('Attribute compress should not be in variable non-coordinate attributes for variable temp. Valid location(s) are [C]',
+                      flat_messages)
+        self.assertIn('Attribute add_offset in variable temp must be a numeric type',
+                      flat_messages)
+
     def test_naming_conventions(self):
         '''
         Section 2.3 Naming Conventions
@@ -185,7 +197,6 @@ class TestCF1_6(BaseTestCase):
         assert len([r for r in results if r.value[0] < r.value[1]]) == 2
         assert all(r.name == u'§2.3 Naming Conventions' for r in results)
 
-
     def test_check_names_unique(self):
         """
         2.3 names should not be distinguished purely by case, i.e., if case is disregarded, no two names should be the same.
@@ -1473,7 +1484,7 @@ class TestCF1_7(BaseTestCase):
             dataset.createVariable('lev', 'd') # dtype=double, dims=1
             dataset.variables['lev'].setncattr('standard_name', 'atmosphere_sigma_coordinate')
             dataset.variables['lev'].setncattr('formula_terms', 'sigma: lev ps: PS ptop: PTOP')
-            
+
             dataset.createVariable('PS', 'd', ('time',)) # dtype=double, dims=time
             dataset.createVariable('PTOP', 'd', ('time',)) # dtype=double, dims=time
 
@@ -1500,7 +1511,7 @@ class TestCF1_7(BaseTestCase):
             # computed_standard_name is assigned, should pass
             score, out_of, messages = get_results(ret_val)
             assert score == out_of
-        
+
     def test_dimensionless_vertical(self):
         '''
         Section 4.3.2 check, but for CF-1.7 implementation. With the refactor in