Automatic reversal of DimCoord bounds (#4466)

* first pass at ensuring contiguous bounds

* reduce scope and fix old tests

* bounds setter tests

* test contiguity preserved

* check bounds within reverse tests

* add comment; merge test

* simpler approach

* revert now redundant changes

* update cml

* cell equality to ignore bound order

* update cml

* move merge test to integration

* review: simple actions

* review: split up cube reverse tests

* review: check bounds in all cube reverse tests

* whatsnew

* review: reduce test repetition

* tweak whatsnew

* blank line

docs/src/whatsnew/dev.rst

@@ -63,6 +63,14 @@ This document explains the changes made to Iris for this release
    cases where the requested point is float and the coordinate has integer
    bounds, reported at :issue:`2969`. (:pull:`4245`)
 
+#. `@rcomer`_ modified bounds setting on :obj:`~iris.coords.DimCoord` instances
+   so that the order of the cell bounds is automatically reversed
+   to match the coordinate's direction if necessary.  This is consistent with
+   the `Bounds for 1-D coordinate variables` subsection of the `Cell Boundaries`_
+   section of the CF Conventions and ensures that contiguity is preserved if a
+   coordinate's direction is reversed. (:issue:`3249`, :issue:`423`,
+   :issue:`4078`, :issue:`3756`, :pull:`4466`)
+
 
 💣 Incompatible Changes
 =======================
@@ -123,4 +131,4 @@ This document explains the changes made to Iris for this release
 .. comment
     Whatsnew resources in alphabetical order:
 
-
+.. _Cell Boundaries: https://cfconventions.org/Data/cf-conventions/cf-conventions-1.9/cf-conventions.html#cell-boundaries

lib/iris/coords.py

@@ -1360,7 +1360,9 @@ def __eq__(self, other):
             else:
                 return self.point == other
         elif isinstance(other, Cell):
-            return (self.point == other.point) and (self.bound == other.bound)
+            return (self.point == other.point) and (
+                self.bound == other.bound or self.bound == other.bound[::-1]
+            )
         elif (
             isinstance(other, str)
             and self.bound is None
@@ -2805,6 +2807,10 @@ def _new_bounds_requirements(self, bounds):
             * bounds are not masked, and
             * bounds are monotonic in the first dimension.
 
+        Also reverse the order of the second dimension if necessary to match the
+        first dimension's direction.  I.e. both should increase or both should
+        decrease.
+
         """
         # Ensure the bounds are a compatible shape.
         if self.shape != bounds.shape[:-1] and not (
@@ -2854,6 +2860,16 @@ def _new_bounds_requirements(self, bounds):
                         emsg.format(self.name(), self.__class__.__name__)
                     )
 
+                if n_bounds == 2:
+                    # Make ordering of bounds consistent with coord's direction
+                    # if possible.
+                    (direction,) = directions
+                    diffs = bounds[:, 0] - bounds[:, 1]
+                    if np.all(np.sign(diffs) == direction):
+                        bounds = np.flip(bounds, axis=1)
+
+        return bounds
+
     @Coord.bounds.setter
     def bounds(self, bounds):
         if bounds is not None:
@@ -2862,8 +2878,9 @@ def bounds(self, bounds):
             # Make sure we have an array (any type of array).
             bounds = np.asanyarray(bounds)
 
-            # Check validity requirements for dimension-coordinate bounds.
-            self._new_bounds_requirements(bounds)
+            # Check validity requirements for dimension-coordinate bounds and reverse
+            # trailing dimension if necessary.
+            bounds = self._new_bounds_requirements(bounds)
             # Cast to a numpy array for masked arrays with no mask.
             bounds = np.array(bounds)
 

lib/iris/tests/integration/merge/__init__.py

@@ -0,0 +1,6 @@
+# Copyright Iris contributors
+#
+# This file is part of Iris and is released under the LGPL license.
+# See COPYING and COPYING.LESSER in the root of the repository for full
+# licensing details.
+"""Integration tests for the :mod:`iris._merge` package."""

lib/iris/tests/integration/merge/test_merge.py

@@ -0,0 +1,37 @@
+# Copyright Iris contributors
+#
+# This file is part of Iris and is released under the LGPL license.
+# See COPYING and COPYING.LESSER in the root of the repository for full
+# licensing details.
+"""
+Integration tests for merging cubes.
+
+"""
+
+# import iris tests first so that some things can be initialised
+# before importing anything else.
+import iris.tests as tests  # isort:skip
+
+from iris.coords import DimCoord
+from iris.cube import Cube, CubeList
+
+
+class TestContiguous(tests.IrisTest):
+    def test_form_contiguous_dimcoord(self):
+        # Test that cube sliced up and remerged in the opposite order maintains
+        # contiguity.
+        cube1 = Cube([1, 2, 3], "air_temperature", units="K")
+        coord1 = DimCoord([3, 2, 1], long_name="spam")
+        coord1.guess_bounds()
+        cube1.add_dim_coord(coord1, 0)
+        cubes = CubeList(cube1.slices_over("spam"))
+        cube2 = cubes.merge_cube()
+        coord2 = cube2.coord("spam")
+
+        self.assertTrue(coord2.is_contiguous())
+        self.assertArrayEqual(coord2.points, [1, 2, 3])
+        self.assertArrayEqual(coord2.bounds, coord1.bounds[::-1, ::-1])
+
+
+if __name__ == "__main__":
+    tests.main()

lib/iris/tests/results/coord_api/intersection_reversed.xml

@@ -1,9 +1,9 @@
 <?xml version="1.0" ?>
-<dimCoord bounds="[[27.75, 30.75],
-		[24.75, 27.75],
-		[21.75, 24.75],
-		[18.75, 21.75],
-		[15.75, 18.75],
-		[12.75, 15.75],
-		[9.75, 12.75],
-		[6.75, 9.75]]" id="43cd7f4a" long_name="foo" points="[30.0, 27.0, 24.0, 21.0, 18.0, 15.0, 12.0, 9.0]" shape="(8,)" units="Unit('meter')" value_type="float32"/>
+<dimCoord bounds="[[30.75, 27.75],
+		[27.75, 24.75],
+		[24.75, 21.75],
+		[21.75, 18.75],
+		[18.75, 15.75],
+		[15.75, 12.75],
+		[12.75, 9.75],
+		[9.75, 6.75]]" id="43cd7f4a" long_name="foo" points="[30.0, 27.0, 24.0, 21.0, 18.0, 15.0, 12.0, 9.0]" shape="(8,)" units="Unit('meter')" value_type="float32"/>

lib/iris/tests/results/cube_slice/2d_intersect_and_reverse.cml

@@ -9,15 +9,15 @@
         <dimCoord id="98bd3c83" long_name="dim1" points="[3.0, 8.1, 13.2, 18.3, 23.4]" shape="(5,)" units="Unit('meters')" value_type="float32"/>
       </coord>
       <coord datadims="[1]">
-        <dimCoord bounds="[[18, 19],
-		[16, 17],
-		[14, 15],
-		[10, 11],
-		[8, 9],
-		[6, 7],
-		[4, 5],
-		[2, 3],
-		[0, 1]]" id="e4dc1958" long_name="dim2" points="[9, 8, 7, 5, 4, 3, 2, 1, 0]" shape="(9,)" units="Unit('meters')" value_type="int32"/>
+        <dimCoord bounds="[[19, 18],
+		[17, 16],
+		[15, 14],
+		[11, 10],
+		[9, 8],
+		[7, 6],
+		[5, 4],
+		[3, 2],
+		[1, 0]]" id="e4dc1958" long_name="dim2" points="[9, 8, 7, 5, 4, 3, 2, 1, 0]" shape="(9,)" units="Unit('meters')" value_type="int32"/>
       </coord>
       <coord datadims="[0, 1]">
         <auxCoord bounds="[[[36, 37, 38, 39],

lib/iris/tests/results/cube_slice/2d_to_2d_revesed.cml

@@ -9,16 +9,16 @@
         <dimCoord id="98bd3c83" long_name="dim1" points="[23.4, 18.3, 13.2, 8.1, 3.0]" shape="(5,)" units="Unit('meters')" value_type="float32"/>
       </coord>
       <coord datadims="[1]">
-        <dimCoord bounds="[[18, 19],
-		[16, 17],
-		[14, 15],
-		[12, 13],
-		[10, 11],
-		[8, 9],
-		[6, 7],
-		[4, 5],
-		[2, 3],
-		[0, 1]]" id="e4dc1958" long_name="dim2" points="[9, 8, 7, 6, 5, 4, 3, 2, 1, 0]" shape="(10,)" units="Unit('meters')" value_type="int32"/>
+        <dimCoord bounds="[[19, 18],
+		[17, 16],
+		[15, 14],
+		[13, 12],
+		[11, 10],
+		[9, 8],
+		[7, 6],
+		[5, 4],
+		[3, 2],
+		[1, 0]]" id="e4dc1958" long_name="dim2" points="[9, 8, 7, 6, 5, 4, 3, 2, 1, 0]" shape="(10,)" units="Unit('meters')" value_type="int32"/>
       </coord>
       <coord datadims="[0, 1]">
         <auxCoord bounds="[[[196, 197, 198, 199],

lib/iris/tests/test_coord_api.py

@@ -49,7 +49,11 @@ def test_slice_multiple_indices(self):
     def test_slice_reverse(self):
         b = self.lat[::-1]
         np.testing.assert_array_equal(b.points, self.lat.points[::-1])
-        np.testing.assert_array_equal(b.bounds, self.lat.bounds[::-1, :])
+        np.testing.assert_array_equal(b.bounds, self.lat.bounds[::-1, ::-1])
+
+        # Check contiguity is preserved.
+        self.assertTrue(self.lat.is_contiguous())
+        self.assertTrue(b.is_contiguous())
 
         c = b[::-1]
         self.assertEqual(self.lat, c)

lib/iris/tests/unit/coords/test_DimCoord.py

@@ -609,6 +609,18 @@ def test_copy_array(self):
         bnds[1, 1] = 10
         self.assertEqual(coord.bounds[1, 1], 5)
 
+    def test_flip_contiguous(self):
+        pts = np.arange(4)
+        bnds = np.transpose([np.arange(1, 5), np.arange(4)])
+        coord = DimCoord(pts, bounds=bnds)
+        self.assertArrayEqual(coord.bounds, bnds[:, ::-1])
+
+    def test_flip_contiguous_decreasing(self):
+        pts = np.arange(4, 0, -1)
+        bnds = np.transpose([np.arange(4, 0, -1), np.arange(5, 1, -1)])
+        coord = DimCoord(pts, bounds=bnds)
+        self.assertArrayEqual(coord.bounds, bnds[:, ::-1])
+
 
 if __name__ == "__main__":
     tests.main()

lib/iris/tests/unit/util/test_reverse.py

@@ -76,10 +76,15 @@ def setUp(self):
         # matching long names but the points array on one cube is reversed
         # with respect to that on the other.
         data = np.arange(12).reshape(3, 4)
+
         self.a1 = iris.coords.DimCoord([1, 2, 3], long_name="a")
+        self.a1.guess_bounds()
         self.b1 = iris.coords.DimCoord([1, 2, 3, 4], long_name="b")
+
         a2 = iris.coords.DimCoord([3, 2, 1], long_name="a")
+        a2.guess_bounds()
         b2 = iris.coords.DimCoord([4, 3, 2, 1], long_name="b")
+
         self.span = iris.coords.AuxCoord(
             np.arange(12).reshape(3, 4), long_name="spanning"
         )
@@ -94,85 +99,93 @@ def setUp(self):
             data, dim_coords_and_dims=[(a2, 0), (b2, 1)]
         )
 
-    def test_cube_dim(self):
-        cube1_reverse0 = reverse(self.cube1, 0)
-        cube1_reverse1 = reverse(self.cube1, 1)
-        cube1_reverse_both = reverse(self.cube1, (0, 1))
-
-        self.assertArrayEqual(self.cube1.data[::-1], cube1_reverse0.data)
+    def check_coorda_reversed(self, result):
         self.assertArrayEqual(
-            self.cube2.coord("a").points, cube1_reverse0.coord("a").points
+            self.cube2.coord("a").points, result.coord("a").points
         )
         self.assertArrayEqual(
-            self.cube1.coord("b").points, cube1_reverse0.coord("b").points
+            self.cube2.coord("a").bounds, result.coord("a").bounds
         )
 
-        self.assertArrayEqual(self.cube1.data[:, ::-1], cube1_reverse1.data)
+    def check_coorda_unchanged(self, result):
         self.assertArrayEqual(
-            self.cube1.coord("a").points, cube1_reverse1.coord("a").points
+            self.cube1.coord("a").points, result.coord("a").points
         )
         self.assertArrayEqual(
-            self.cube2.coord("b").points, cube1_reverse1.coord("b").points
+            self.cube1.coord("a").bounds, result.coord("a").bounds
         )
 
+    def check_coordb_reversed(self, result):
         self.assertArrayEqual(
-            self.cube1.data[::-1, ::-1], cube1_reverse_both.data
+            self.cube2.coord("b").points, result.coord("b").points
         )
+
+    def check_coordb_unchanged(self, result):
         self.assertArrayEqual(
-            self.cube2.coord("a").points, cube1_reverse_both.coord("a").points
+            self.cube1.coord("b").points, result.coord("b").points
         )
+
+    def test_cube_dim0(self):
+        cube1_reverse0 = reverse(self.cube1, 0)
+
+        self.assertArrayEqual(self.cube1.data[::-1], cube1_reverse0.data)
+        self.check_coorda_reversed(cube1_reverse0)
+        self.check_coordb_unchanged(cube1_reverse0)
+
+    def test_cube_dim1(self):
+        cube1_reverse1 = reverse(self.cube1, 1)
+
+        self.assertArrayEqual(self.cube1.data[:, ::-1], cube1_reverse1.data)
+        self.check_coordb_reversed(cube1_reverse1)
+        self.check_coorda_unchanged(cube1_reverse1)
+
+    def test_cube_dim_both(self):
+        cube1_reverse_both = reverse(self.cube1, (0, 1))
+
         self.assertArrayEqual(
-            self.cube2.coord("b").points, cube1_reverse_both.coord("b").points
+            self.cube1.data[::-1, ::-1], cube1_reverse_both.data
         )
+        self.check_coorda_reversed(cube1_reverse_both)
+        self.check_coordb_reversed(cube1_reverse_both)
 
-    def test_cube_coord(self):
+    def test_cube_coord0(self):
         cube1_reverse0 = reverse(self.cube1, self.a1)
-        cube1_reverse1 = reverse(self.cube1, "b")
-        cube1_reverse_both = reverse(self.cube1, (self.a1, self.b1))
-        cube1_reverse_spanning = reverse(self.cube1, "spanning")
 
         self.assertArrayEqual(self.cube1.data[::-1], cube1_reverse0.data)
-        self.assertArrayEqual(
-            self.cube2.coord("a").points, cube1_reverse0.coord("a").points
-        )
-        self.assertArrayEqual(
-            self.cube1.coord("b").points, cube1_reverse0.coord("b").points
-        )
+        self.check_coorda_reversed(cube1_reverse0)
+        self.check_coordb_unchanged(cube1_reverse0)
+
+    def test_cube_coord1(self):
+        cube1_reverse1 = reverse(self.cube1, "b")
 
         self.assertArrayEqual(self.cube1.data[:, ::-1], cube1_reverse1.data)
-        self.assertArrayEqual(
-            self.cube1.coord("a").points, cube1_reverse1.coord("a").points
-        )
-        self.assertArrayEqual(
-            self.cube2.coord("b").points, cube1_reverse1.coord("b").points
-        )
+        self.check_coordb_reversed(cube1_reverse1)
+        self.check_coorda_unchanged(cube1_reverse1)
+
+    def test_cube_coord_both(self):
+        cube1_reverse_both = reverse(self.cube1, (self.a1, self.b1))
 
         self.assertArrayEqual(
             self.cube1.data[::-1, ::-1], cube1_reverse_both.data
         )
-        self.assertArrayEqual(
-            self.cube2.coord("a").points, cube1_reverse_both.coord("a").points
-        )
-        self.assertArrayEqual(
-            self.cube2.coord("b").points, cube1_reverse_both.coord("b").points
-        )
+        self.check_coorda_reversed(cube1_reverse_both)
+        self.check_coordb_reversed(cube1_reverse_both)
+
+    def test_cube_coord_spanning(self):
+        cube1_reverse_spanning = reverse(self.cube1, "spanning")
 
         self.assertArrayEqual(
             self.cube1.data[::-1, ::-1], cube1_reverse_spanning.data
         )
-        self.assertArrayEqual(
-            self.cube2.coord("a").points,
-            cube1_reverse_spanning.coord("a").points,
-        )
-        self.assertArrayEqual(
-            self.cube2.coord("b").points,
-            cube1_reverse_spanning.coord("b").points,
-        )
+        self.check_coorda_reversed(cube1_reverse_spanning)
+        self.check_coordb_reversed(cube1_reverse_spanning)
+
         self.assertArrayEqual(
             self.span.points[::-1, ::-1],
             cube1_reverse_spanning.coord("spanning").points,
         )
 
+    def test_wrong_coord_name(self):
         msg = (
             "Expected to find exactly 1 'latitude' coordinate, but found none."
         )
@@ -181,17 +194,20 @@ def test_cube_coord(self):
         ):
             reverse(self.cube1, "latitude")
 
+    def test_empty_list(self):
         msg = "Reverse was expecting a single axis or a 1d array *"
         with self.assertRaisesRegex(ValueError, msg):
             reverse(self.cube1, [])
 
+    def test_wrong_type_cube(self):
         msg = (
             "coords_or_dims must be int, str, coordinate or sequence of "
             "these.  Got cube."
         )
         with self.assertRaisesRegex(TypeError, msg):
             reverse(self.cube1, self.cube1)
 
+    def test_wrong_type_float(self):
         msg = (
             "coords_or_dims must be int, str, coordinate or sequence of "
             "these."