diff --git a/Changelog.rst b/Changelog.rst index 546aa97ecf..d04f1a6244 100644 --- a/Changelog.rst +++ b/Changelog.rst @@ -27,6 +27,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) diff --git a/cf/aggregate.py b/cf/aggregate.py index 39f2c9bfa1..380a87e66b 100644 --- a/cf/aggregate.py +++ b/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 diff --git a/cf/test/test_aggregate.py b/cf/test/test_aggregate.py index 7093e902e9..53c0b9b938 100644 --- a/cf/test/test_aggregate.py +++ b/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()) diff --git a/docs/source/contributing.rst b/docs/source/contributing.rst index 786fe047e2..def33598e8 100644 --- a/docs/source/contributing.rst +++ b/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