Repository: ioos/compliance-checker
Pull request #520 (2017-09-25 14:32:08)

Cell methods improvements
Improve checking of cell methods contents which appear inside parentheses.  Currently these include standalone comments (no colon), interval specifications, and comments coming after standard elements.  Also checks if the element with a colon even exists and throws an error if it does not.


Patch:
diff --git a/compliance_checker/cf/cf.py b/compliance_checker/cf/cf.py
index a3512f9..3a5d287 100644
--- a/compliance_checker/cf/cf.py
+++ b/compliance_checker/cf/cf.py
@@ -2635,6 +2635,7 @@ class CFBaseCheck(BaseCheck):
 
         return ret_val
 
+
     def check_cell_methods(self, ds):
         """
         7.3 To describe the characteristic of a field that is represented by cell values, we define the cell_methods attribute
@@ -2673,9 +2674,8 @@ class CFBaseCheck(BaseCheck):
         }
 
         ret_val = []
-        psep = regex.compile(r'((?P<vars>\w+: )+(?P<method>\w+) ?(?P<where>where (?P<wtypevar>\w+) '
-                              '?(?P<over>over (?P<otypevar>\w+))?| ?)(?P<brace>\(((?P<brace_wunit>\w+): '
-                              '(?P<interval_val>\w+) (?P<interval_unit>\w+)|(?P<brace_opt>\w+): (\w+))\))?)*')
+        psep = regex.compile(r'(?P<vars>\w+: )+(?P<method>\w+) ?(?P<where>where (?P<wtypevar>\w+) '
+                             '?(?P<over>over (?P<otypevar>\w+))?| ?)(?:\((?P<paren_contents>[^)]*)\))?')
 
         for var in ds.get_variables_by_attributes(cell_methods=lambda x: x is not None):
             if not getattr(var, 'cell_methods', ''):
@@ -2719,44 +2719,95 @@ class CFBaseCheck(BaseCheck):
                                          '§7.3 {} has valid methods in cell_methods attribute'.format(var.name))
 
             for match in regex.finditer(psep, method):
-                valid_cell_methods.assert_true(match.group('method') in methods,
+                # CF section 7.3 - "Case is not significant in the method name."
+                valid_cell_methods.assert_true(match.group('method').lower() in methods,
                                                '{}:cell_methods contains an invalid method: {}'
                                                ''.format(var.name, match.group('method')))
 
             ret_val.append(valid_cell_methods.to_result())
 
-            valid_modifier = TestCtx(BaseCheck.MEDIUM,
-                                     '§7.3.3 {} has valid cell_methods modifiers'.format(var.name))
-
             for match in regex.finditer(psep, method):
-                if match.group('brace') is not None:
-                    valid_modifier.assert_true(match.group('brace_wunit') in {'interval', 'comment', 'area'},
-                                               '{}:cell_methods contains an invalid modifier: {}. It should be one '
-                                               'of interval, comment or area.'
-                                               ''.format(var.name, match.group('brace_wunit')))
-                    if match.group('brace_wunit') == 'interval':
-                        # attempt to get the number for the interval
-                        valid_modifier.out_of += 2
-                        try:
-                            float(match.group('interval_val'))
-                        except ValueError:
-                            valid_modifier.messages.append('{}:cell_methods contains an interval value that does not parse as a numeric value: "{}".'.format(var.name, match.group('interval_val')))
-                        else:
-                            valid_modifier.score += 1
-
-                        # then the units
-                        try:
-                            Unit(match.group('interval_unit'))
-                        except ValueError:
-                            valid_modifier.messages.append('{}:cell_methods interval units "{}" is not parsable by UDUNITS.'.format(var.name, match.group('interval_unit')))
-                        else:
-                            valid_modifier.score += 1
-
-            if valid_modifier.out_of > 0:
-                ret_val.append(valid_modifier.to_result())
+                if match.group('paren_contents') is not None:
+                    # split along spaces followed by words with a colon
+                    # not sure what to do if a comment contains a colon!
+                    ret_val.append(self._check_cell_methods_paren_info(match.group('paren_contents'), var).to_result())
 
         return ret_val
 
+    def _check_cell_methods_paren_info(self, paren_contents, var):
+        """
+        Checks that the spacing and/or comment info contained inside the
+        parentheses in cell_methods is well-formed
+        """
+        #valid_info = TestCtx(BaseCheck.MEDIUM,
+        #                            '§7.3.2 {} has valid cell_methods spacing/other info'.format(var.name))
+        valid_info = TestCtx(BaseCheck.MEDIUM,
+                             '§7.3.3 {} has valid cell_methods modifiers'.format(var.name))
+        # if there are no colons, this is a simple comment
+        # TODO: are empty comments considered valid?
+        if ':' not in paren_contents:
+            valid_info.out_of += 1
+            valid_info.score += 1
+            return valid_info
+        # otherwise, split into k/v pairs
+        kv_pair_pat = r'(\S+:)\s+(.*(?=\s+\w+:)|[^:]+$)\s*'
+        # otherwise, we must split further with intervals coming
+        # first, followed by non-standard comments
+        # we need the count of the matches, and re.findall() only returns
+        # groups if they are present and we wish to see if the entire match
+        # object concatenated together is the same as the original string
+        pmatches = [m for m in regex.finditer(kv_pair_pat, paren_contents)]
+        for i, pmatch in enumerate(pmatches):
+            keyword, val = pmatch.groups()
+            if keyword == 'interval:':
+                valid_info.out_of += 2
+                interval_matches = regex.match(r'^\s*(?P<interval_number>\S+)\s+(?P<interval_units>\S+)\s*$', val)
+                # attempt to get the number for the interval
+                if not interval_matches:
+                    valid_info.messages.append('{}:cell_methods contains an interval specification that does not parse: "{}". Should be in format "interval: <number> <units>"'.format(var.name, val))
+                else:
+                    try:
+                        float(interval_matches.group('interval_number'))
+                    except ValueError:
+                        valid_info.messages.append('{}:cell_methods contains an interval value that does not parse as a numeric value: "{}".'.format(var.name, interval_matches.group('interval_number')))
+                    else:
+                        valid_info.score += 1
+
+                    # then the units
+                    try:
+                        Unit(interval_matches.group('interval_units'))
+                    except ValueError:
+                        valid_info.messages.append('{}:cell_methods interval units "{}" is not parsable by UDUNITS.'.format(var.name, interval_matches.group('interval_units')))
+                    else:
+                        valid_info.score += 1
+            elif keyword == 'comment:':
+                # comments can't really be invalid, except
+                # if they come first or aren't last, and
+                # maybe if they contain colons embedded in the
+                # comment string
+                valid_info.out_of += 1
+                if len(pmatches) == 1:
+                    valid_info.messages.append('If there is no standardized information, the keyword comment: should be omitted for variable {}'.format(var.name))
+                # otherwise check that the comment is the last
+                # item in the parentheses
+                elif i != len(pmatches) - 1:
+                    valid_info.messages.append('The non-standard "comment:" element must come after any standard elements in cell_methods for variable {}'.format(var.name))
+                #
+                else:
+                    valid_info.score += 1
+            else:
+                valid_info.out_of += 1
+                valid_info.messages.append('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)
+                                       for m in pmatches) == paren_contents,
+                   "Parenthetical content inside cell_methods is not well formed: {}".format(paren_contents))
+
+        return valid_info
+
     def check_climatological_statistics(self, ds):
         """
         7.4 A climatological time coordinate variable does not have a bounds attribute. Instead, it has a climatology


Test patch:
diff --git a/compliance_checker/tests/test_cf.py b/compliance_checker/tests/test_cf.py
index 00283c8..1b87435 100644
--- a/compliance_checker/tests/test_cf.py
+++ b/compliance_checker/tests/test_cf.py
@@ -983,6 +983,25 @@ class TestCF(BaseTestCase):
                         in messages)
         self.assertTrue('temperature:cell_methods interval units "whizbangs" is not parsable by UDUNITS.'
                         in messages)
+        temp.cell_methods = 'lat: lon: mean depth: mean (comment: should not go here interval: 2.5 m)'
+        results = self.cf.check_cell_methods(nc_obj)
+        self.assertTrue('The non-standard "comment:" element must come after any standard elements in cell_methods for variable temperature')
+        # standalone comments require no keyword
+        temp.cell_methods = 'lon: mean (This is a standalone comment)'
+        results = self.cf.check_cell_methods(nc_obj)
+        result_dict = {result.name: result for result in results}
+        modifier_results = result_dict[u'§7.3.3 temperature has valid cell_methods modifiers']
+        self.assertTrue(modifier_results.value == (1, 1))
+        temp.cell_methods = 'lat: lon: mean depth: mean (invalid_keyword: this is invalid)'
+        results = self.cf.check_cell_methods(nc_obj)
+        self.assertTrue('Invalid cell_methods keyword "invalid_keyword" for variable temperature. Must be one of [interval, comment]')
+        temp.cell_methods = 'lat: lon: mean depth: mean (interval: 0.2 m comment: This should come last interval: 0.01 degrees)'
+        results = self.cf.check_cell_methods(nc_obj)
+        self.assertTrue('The non-standard "comment:" element must come after any standard elements in cell_methods for variable temperature')
+        temp.cell_methods = 'lat: lon: mean depth: mean (interval 0.2 m interval: 0.01 degrees)'
+        results = self.cf.check_cell_methods(nc_obj)
+        self.assertTrue('Parenthetical content inside cell_methods is not well formed: interval 0.2 m interval: 0.01 degrees')
+
 
 
     # --------------------------------------------------------------------------------