Merge pull request #765 from davidhassell/aggregation-valid-range

Fix `cf.aggregate` for actual_range and np.array properties

Changelog.rst

@@ -32,6 +32,12 @@ version 3.16.2
 * Fix bug in `cf.aggregate` that sometimes put a null transpose
   operation into the Dask graph when one was not needed
   (https://github.com/NCAS-CMS/cf-python/issues/754)
+* Fix bug in `cf.aggregate` that caused a failure when property values
+  were `numpy` arrays with two or more elements
+  (https://github.com/NCAS-CMS/cf-python/issues/764)
+* Fix bug in `cf.aggregate` that didn't correctly handle the
+  "actual_range" CF attribute
+  (https://github.com/NCAS-CMS/cf-python/issues/764)
 * Fix bug whereby `Field.cyclic` is not updated after a
   `Field.del_construct` operation
   (https://github.com/NCAS-CMS/cf-python/issues/758)

cf/aggregate.py

@@ -4852,20 +4852,36 @@ def _aggregate_2_fields(
         value0 = parent0.get_property(prop, None)
         value1 = parent1.get_property(prop, None)
 
+        if prop in ("_FillValue", "missing_value"):
+            continue
+
         if prop in ("valid_min", "valid_max", "valid_range"):
             if not m0.respect_valid:
                 parent0.del_property(prop, None)
 
             continue
 
-        if prop in ("_FillValue", "missing_value"):
+        if prop == "actual_range":
+            try:
+                # Try to extend the actual range to encompass both
+                # value0 and value1
+                actual_range = (
+                    min(value0[0], value1[0]),
+                    max(value0[1], value1[1]),
+                )
+            except (TypeError, IndexError, KeyError):
+                # value0 and/or value1 is not set, or is
+                # non-CF-compliant.
+                parent0.del_property(prop, None)
+            else:
+                parent0.set_property(prop, actual_range)
+
             continue
 
         # Still here?
-        if isinstance(value0, str) or isinstance(value1, str):
-            if value0 == value1:
-                continue
-        elif parent0._equals(value0, value1):
+        if parent0._equals(value0, value1):
+            # Both values are equal, so no need to update the
+            # property.
             continue
 
         if concatenate:
@@ -4876,9 +4892,30 @@ def _aggregate_2_fields(
                     )
                 else:
                     parent0.set_property(prop, f" :AGGREGATED: {value1}")
-        else:
-            if value0 is not None:
-                parent0.del_property(prop)
+        elif value0 is not None:
+            parent0.del_property(prop)
+
+    # Check that actual_range is within the bounds of valid_range, and
+    # delete it if it isn't.
+    actual_range = parent0.get_property("actual_range", None)
+    if actual_range is not None:
+        valid_range = parent0.get_property("valid_range", None)
+        if valid_range is not None:
+            try:
+                if (
+                    actual_range[0] < valid_range[0]
+                    or actual_range[1] > valid_range[1]
+                ):
+                    actual_range = parent0.del_property("actual_range", None)
+                    if actual_range is not None and is_log_level_info(logger):
+                        logger.info(
+                            "Deleted 'actual_range' attribute due to being "
+                            "outside of 'valid_range' attribute limits."
+                        )
+
+            except (TypeError, IndexError):
+                # valid_range is non-CF-compliant
+                pass
 
     # Make a note that the parent construct in this _Meta object has
     # already been aggregated

cf/test/test_aggregate.py

@@ -4,6 +4,8 @@
 import unittest
 import warnings
 
+import numpy as np
+
 faulthandler.enable()  # to debug seg faults and timeouts
 
 import cf
@@ -664,6 +666,77 @@ def test_aggregate_trajectory(self):
             )
         )
 
+    def test_aggregate_actual_range(self):
+        """Test aggregation of actual_range"""
+        f = cf.example_field(0)
+        f.set_property("actual_range", (5, 10))
+        f.set_property("valid_range", (0, 15))
+        f0 = f[:, :2]
+        f1 = f[:, 2:4]
+        f2 = f[:, 4:]
+
+        g = cf.aggregate([f0, f1, f2])
+        self.assertEqual(len(g), 1)
+        self.assertEqual(g[0].get_property("actual_range"), (5, 10))
+
+        f1.set_property("actual_range", [2, 13])
+        g = cf.aggregate([f0, f1, f2])
+        self.assertEqual(len(g), 1)
+        self.assertEqual(g[0].get_property("actual_range"), (2, 13))
+
+        f1.set_property("actual_range", [-2, 17])
+        g = cf.aggregate([f0, f1, f2])
+        self.assertEqual(len(g), 1)
+        self.assertEqual(g[0].get_property("actual_range"), (-2, 17))
+
+        g = cf.aggregate([f0, f1, f2], respect_valid=True)
+        self.assertEqual(len(g), 1)
+        self.assertEqual(g[0].get_property("valid_range"), (0, 15))
+        self.assertFalse(g[0].has_property("actual_range"))
+
+        f1.set_property("actual_range", [0, 15])
+        g = cf.aggregate([f0, f1, f2], respect_valid=True)
+        self.assertEqual(len(g), 1)
+        self.assertEqual(g[0].get_property("valid_range"), (0, 15))
+        self.assertEqual(g[0].get_property("actual_range"), (0, 15))
+
+    def test_aggregate_numpy_array_property(self):
+        """Test aggregation of numpy array-valued properties"""
+        a = np.array([5, 10])
+        f = cf.example_field(0)
+        f.set_property("array", a)
+        f0 = f[:, :2]
+        f1 = f[:, 2:4]
+        f2 = f[:, 4:]
+
+        g = cf.aggregate([f0, f1, f2])
+        self.assertEqual(len(g), 1)
+        self.assertTrue((g[0].get_property("array") == a).all())
+
+        f1.set_property("array", np.array([-5, 20]))
+        g = cf.aggregate([f0, f1, f2])
+        self.assertEqual(len(g), 1)
+        self.assertEqual(
+            g[0].get_property("array"),
+            "[ 5 10] :AGGREGATED: [-5 20] :AGGREGATED: [ 5 10]",
+        )
+
+        f2.set_property("array", np.array([-5, 20]))
+        g = cf.aggregate([f0, f1, f2])
+        self.assertEqual(len(g), 1)
+        self.assertEqual(
+            g[0].get_property("array"),
+            "[ 5 10] :AGGREGATED: [-5 20] :AGGREGATED: [-5 20]",
+        )
+
+        f1.set_property("array", np.array([5, 10]))
+        g = cf.aggregate([f0, f1, f2])
+        self.assertEqual(len(g), 1)
+        self.assertEqual(
+            g[0].get_property("array"),
+            "[ 5 10] :AGGREGATED: [-5 20]",
+        )
+
 
 if __name__ == "__main__":
     print("Run date:", datetime.datetime.now())

docs/source/contributing.rst

@@ -121,6 +121,7 @@ ideas, code, and documentation to the cf library:
 * Jonathan Gregory
 * Klaus Zimmermann
 * Kristian Sebastián
+* Mark Rhodes-Smith
 * Michael Decker
 * Sadie Bartholomew
 * Thibault Hallouin