Merge pull request #810 from davidhassell/timeduration-mod

Fix failure when creating 'T: mean within days time: minimum over days'' collapse

Author: davidhassell

Changelog.rst

@@ -9,6 +9,8 @@ version NEXTVERSION
   (https://github.com/NCAS-CMS/cf-python/issues/784)
 * Include the UM version as a field property when reading UM files
   (https://github.com/NCAS-CMS/cf-python/issues/777)
+* Fix bug that caused climatological time collapses within/over days
+  to fail (https://github.com/NCAS-CMS/cf-python/issues/809)
 * New keyword parameter to `cf.Field.derivative`:
   ``ignore_coordinate_units``
   (https://github.com/NCAS-CMS/cf-python/issues/807)

cf/field.py

@@ -8392,7 +8392,6 @@ def _group_weights(weights, iaxis, index):
                         within_days.Units.istime
                         and TimeDuration(24, "hours") % within_days
                     ):
-                        # % Data(1, 'day'): # % within_days:
                         raise ValueError(
                             f"Can't collapse: within_days={within_days!r} "
                             "is not an exact factor of 1 day"

cf/test/test_TimeDuration.py

@@ -107,7 +107,18 @@ def test_TimeDuration(self):
         self.assertEqual(
             cf.TimeDuration(36, "calendar_months") // 8.25, cf.M(4.0)
         )
-        self.assertEqual(cf.TimeDuration(36, "calendar_months") % 10, cf.M(6))
+
+        for y in (
+            10,
+            cf.Data(10, "calendar_months"),
+            cf.TimeDuration(10, "calendar_months"),
+        ):
+            self.assertEqual(
+                cf.TimeDuration(36, "calendar_months") % y, cf.M(6)
+            )
+
+        for y in (10, cf.Data(10, "hours"), cf.h(10), cf.D(5 / 12), cf.m(600)):
+            self.assertEqual(cf.h(36) % y, cf.h(6))
 
         self.assertEqual(
             cf.TimeDuration(24, "hours") + cf.TimeDuration(0.5, "days"),

cf/test/test_collapse.py

@@ -246,6 +246,23 @@ def test_Field_collapse_CLIMATOLOGICAL_TIME(self):
             print(g.constructs)
         self.assertEqual(list(g.shape), expected_shape)
 
+    def test_Field_collapse_CLIMATOLOGICAL_TIME_within_days(self):
+        verbose = False
+
+        f = cf.example_field(5)
+
+        g = f.collapse(
+            "T: mean within days time: minimum over days", within_days=cf.h(12)
+        )
+        expected_shape = list(f.shape)
+        expected_shape[0] = 2
+
+        if verbose:
+            print("\n", f)
+            print(g)
+            print(g.constructs)
+        self.assertEqual(list(g.shape), expected_shape)
+
     def test_Field_collapse(self):
         verbose = False
 

cf/timeduration.py

@@ -696,7 +696,6 @@ def _apply_binary_comparison(self, other, op):
             op: `str`, the binary comparison operator to apply.
 
         """
-
         if isinstance(other, (self.__class__, int, float)):
             return bool(self._binary_operation(other, op))
 
@@ -737,15 +736,14 @@ def _apply_binary_arithmetic(
                 do not skip.
 
         """
-        check_simple_types = [int, float]
-        if may_be_datetime:
-            check_simple_types.append(self.__class__)
-
-        if isinstance(other, tuple(check_simple_types)):
+        if isinstance(other, (int, float, self.__class__)):
+            # 'other' is a number or another TimeDuration object => we
+            # can use the usual binary operation method.
             return self._binary_operation(other, op, aug_assignment)
 
         if may_be_datetime and hasattr(other, "timetuple"):
-            # other is a date-time object
+            # 'other' is a date-time object => we must use the special
+            # datetime arithmetic operation.
             try:
                 return self._datetime_arithmetic(other, getattr(operator, op))
             except TypeError: