Added additional validation for connectivity checks. #168
Conversation
| try: | ||
| fill_value = data_array.encoding['_FillValue'] | ||
| except KeyError: | ||
| return True |
There was a problem hiding this comment.
Because of the nature of check functions like this, an early return of True can lead to bugs in the future. If another developer comes along and adds another check below this check - just as you've added a check below the existing checks - then that new check will be skipped if there is a missing _FillValue. An early return of False is always valid as any failed check renders the whole connectivity array invalid. Consider instead:
if '_FillValue' in data_array.encoding:
fill_value = data_array.encoding['_FillValue']
...
if lower_bound < fill_value < upper_bound:
warnings.warn(...)
return False
return TrueNew checks can be safely added later by appending them below the existing set of checks without risk of the check being skipped.
| if lower_bound < fill_value < theoretical_upper_bound: | ||
| warnings.warn( | ||
| f"Got a face_edge_connectivity variable {data_array.name!r} with " | ||
| f"a _FillValue inside the theoretical index range", | ||
| ConventionViolationWarning, | ||
| ) | ||
| return False |
There was a problem hiding this comment.
This shouldn't be checked. The theoretical upper bound is a conservative over estimate, useful when computing our own _FillValue as a value that is guaranteed never to collide. However in practice lower values can safely be used as long as the actual _FillValue is lower than actual upper bound. This test will give superfluous warnings on valid data sets.
|
|
||
| lower_bound = _get_start_index(data_array) | ||
| theoretical_upper_bound = self.face_count * self.max_node_count | ||
| actual_upper_bound = numpy.nanmax(data_array) |
There was a problem hiding this comment.
The maximum edge ID can be found more precisely with
upper_bound = self.edge_count + lower_boundBecause of the exact issue of variables being incorrectly masked and therefore missing valid data, any information gleaned by introspecting the data is potentially suspect. This check as written would fail if _FillValue was exactly the maximum edge ID. numpy.nanmax() would not find this value as it has been masked, returning the second-to-last value which would then incorrectly cause the checks below to pass.
| theoretical_upper_bound = self.face_count * self.max_node_count | ||
| actual_upper_bound = numpy.nanmax(data_array) | ||
|
|
||
| if lower_bound < fill_value < actual_upper_bound: |
There was a problem hiding this comment.
These should be less than or equal checks. A fill value of 0 and a lower bound of 0 is invalid as it will mask out the first edge ID:
if lower_bound <= fill_value <= upper_bound:
Starting with just edge face connectivity to make sure were on the right track.