Investigate and reduce benchmark changes for mesh timestamps (#6549)

* tests failing, pushing up for testing.

* Testing a theory

* Testing a theory

* Hopefully, this should maintain benchmark improvements and return functionality

* here's hoping it's that simple

* here's hoping it's that simple

* removed unnecessary timestamp.update()

* self review comments

* made update_from_mesh() force an update, even if timestamps appear alligned

* Update components.py

* Update test_MeshCoord.py

Author: ESadek-MO

lib/iris/coords.py

@@ -573,8 +573,9 @@ def reindent_data_string(text, n_indent):
 
     def __setattr__(self, key, value):
         if getattr(self, "_mesh_timestamps", None) is not None:
-            for timestamp in self._mesh_timestamps:
-                timestamp.update()
+            if key in ("points", "bounds", "_values", "indices"):
+                for timestamp in self._mesh_timestamps:
+                    timestamp.update()
         object.__setattr__(self, key, value)
 
     def __str__(self):

lib/iris/mesh/components.py

@@ -2481,7 +2481,6 @@ def all_members(self):
 
     @property
     def _members(self):
-        self.timestamp.update()
         return self._members_dict
 
     @_members.setter
@@ -2776,6 +2775,7 @@ def __init__(
         self._last_modified = None
         self._updating = True
         self._read_only = True
+
         # Setup the metadata.
         self._metadata_manager_temp = metadata_manager_factory(MeshCoordMetadata)
 
@@ -2820,20 +2820,25 @@ def __init__(
             raise e
         finally:
             self._read_only = True
+        self._last_modified = self.mesh._last_modified
         self._updating = False
 
     def __getattribute__(self, item):
-        # Ensure that the MeshCoord is up to date whenever you use it
+        # Ensure that the MeshCoord is up to date each time you access an attribute.
         # object.__getattribute__ bypasses this block to avoid infinite recursion
-        # by calling the method on the base class `object`
-        updating = object.__getattribute__(self, "_updating")
-        if updating is False and item != "update_from_mesh":
+        # by calling the method on the base class `object`.
+        mesh = object.__getattribute__(self, "_mesh")
+        mesh_last_modified = object.__getattribute__(mesh, "_last_modified")
+        self_last_modified = object.__getattribute__(self, "_last_modified")
+        if (
+            self_last_modified is None or self_last_modified < mesh_last_modified
+        ) and item != "update_from_mesh":
             object.__getattribute__(self, "update_from_mesh")()
         return super().__getattribute__(item)
 
-        # Define accessors for MeshCoord-specific properties mesh/location/axis.
+    # Define accessors for MeshCoord-specific properties mesh/location/axis, and
+    # ensure every property on a MeshCoord is read-only.
 
-    # These are all read-only.
     @property
     def mesh(self):
         return self._mesh
@@ -2943,7 +2948,8 @@ def climatological(self, value):
     def points(self):
         """The coordinate points values as a NumPy array."""
         try:
-            # the points property should return real data, but shouldn't realise the coord on the mesh
+            # The points property should return real data, but shouldn't
+            # realise the coord on the mesh.
             return super().core_points().compute()
         except AttributeError:
             return super().core_points()
@@ -2957,6 +2963,8 @@ def points(self, value):
 
     @property
     def bounds(self):
+        # The bounds property should return real data, but shouldn't
+        # realise the coord on the mesh.
         try:
             return super().core_bounds().compute()
         except AttributeError:
@@ -2973,9 +2981,8 @@ def bounds(self, value):
 
     @property
     def _metadata_manager(self):
-        # sets the metadata
+        # Fetches the metadata from the mesh every time any metadata is accessed.
         use_metadict = self._load_metadata()
-
         self._metadata_manager_temp.standard_name = use_metadict["standard_name"]
         self._metadata_manager_temp.long_name = use_metadict["long_name"]
         self._metadata_manager_temp.var_name = use_metadict["var_name"]
@@ -2999,22 +3006,27 @@ def __getitem__(self, keys):
         return self.copy()
 
     def update_from_mesh(self):
-        try:
-            object.__setattr__(self, "_updating", True)
-            if (self._last_modified is None) or (
-                self._last_modified < self.mesh._last_modified
-            ):
+        """Fetch and recalculate the points and bounds from the relevant coord on the mesh.
+
+        In most cases, updates should be done automatically, but this method can be used
+        if for some reason the points or bounds are out of date.
+        """
+        updating = object.__getattribute__(self, "_updating")
+        if updating is False:
+            try:
+                object.__setattr__(self, "_updating", True)
+                # update points and bounds
                 points, bounds = self._load_points_and_bounds()
                 super(MeshCoord, self.__class__).points.fset(self, points)
                 super(MeshCoord, self.__class__).bounds.fset(self, bounds)
                 object.__setattr__(self, "_last_modified", self.mesh._last_modified)
-        # Ensure errors aren't bypassed
-        except Exception as e:
-            raise e
-        finally:
-            # if _updating isn't reset, this would mean the MeshCoord would never
-            # update from the attached Mesh, breaking the link
-            object.__setattr__(self, "_updating", False)
+            # Ensure errors aren't bypassed
+            except Exception as e:
+                raise e
+            finally:
+                # if _updating isn't reset, this would mean the MeshCoord would never
+                # update from the attached Mesh, breaking the link
+                object.__setattr__(self, "_updating", False)
 
     def collapsed(self, dims_to_collapse=None):
         """Return a copy of this coordinate, which has been collapsed along the specified dimensions.
@@ -3188,6 +3200,7 @@ def _load_points_and_bounds(self):
             #  extra work to refactor the parent classes.
             msg = "Cannot yet create a MeshCoord without points."
             raise ValueError(msg)
+
         return points, bounds
 
     def _load_metadata(self):

lib/iris/tests/unit/mesh/components/test_MeshCoord.py

@@ -1030,33 +1030,42 @@ def _set_all_coords(self, name, value):
             c.__setattr__(name, value)
 
     def test__last_modified(self):
-        # Ensure they are identical at creation.
+        # Ensure that mesh._last_modified is updated when you update the mesh, and
+        # and meshcoord._last_modified is updated to match that.
+
         assert self.meshcoord.mesh._last_modified == self.timestamp_at_creation
 
         self.coord_on_mesh.points = np.zeros(3)
 
         assert self.meshcoord.mesh._last_modified == self.meshcoord._last_modified
         assert self.meshcoord.mesh._last_modified != self.timestamp_at_creation
 
-        self.meshcoord.standard_name
-
-        assert self.meshcoord.mesh._last_modified == self.meshcoord._last_modified
-        assert self.meshcoord.mesh._last_modified != self.timestamp_at_creation
-
-    def test_points(self):
+    def test_points(self, mocker):
         zeroes = np.zeros(3)
+        mocked = mocker.patch.object(MeshCoord, "_load_points_and_bounds")
+        mocked.return_value = (
+            zeroes,
+            np.zeros((3, 3)),
+        )
         assert self.meshcoord.points.all() != zeroes.all()
         self.coord_on_mesh.points = zeroes
         assert self.meshcoord.points.all() == zeroes.all()
+        mocked.assert_called_once()
 
-    def test_bounds(self):
+    def test_bounds(self, mocker):
         zero_bounds = np.zeros(3)
         zero_points = np.zeros(15)
+        mocked = mocker.patch.object(MeshCoord, "_load_points_and_bounds")
+        mocked.return_value = (
+            zero_bounds,
+            np.zeros((3, 3)),
+        )
         assert self.meshcoord.bounds.all() != zero_bounds.all()
         # Node coords are used to calculate the meshcoord bounds
         for nc in self.meshcoord.mesh.node_coords:
             nc.points = zero_points
         assert self.meshcoord.bounds.all() == zero_bounds.all()
+        mocked.assert_called_once()
 
     @pytest.mark.parametrize(
         "metadata_name, value",
@@ -1066,23 +1075,37 @@ def test_bounds(self):
             ("attributes", {"foo": 1}),
         ],
     )
-    def test_basic_metadata(self, metadata_name, value):
+    def test_basic_metadata(self, metadata_name, value, mocker):
+        mocked = mocker.patch.object(MeshCoord, "_load_points_and_bounds")
+
         self.coord_on_mesh.__setattr__(metadata_name, value)
         assert self.meshcoord.__getattribute__(
             metadata_name
         ) == self.coord_on_mesh.__getattribute__(metadata_name)
 
-    def test_units(self):
+        # Ensure updating metadata doesn't prompt the MeshCoord
+        # to update points and bounds.
+        mocked.assert_not_called()
+
+    def test_units(self, mocker):
+        mocked = mocker.patch.object(MeshCoord, "_load_points_and_bounds")
         for c in self.mesh.coords():
             c.units = "radians"
         assert self.meshcoord.standard_name == self.coord_on_mesh.standard_name
+        # Ensure updating metadata doesn't prompt the MeshCoord
+        # to update points and bounds.
+        mocked.assert_not_called()
 
-    def test_standard_name(self):
+    def test_standard_name(self, mocker):
+        mocked = mocker.patch.object(MeshCoord, "_load_points_and_bounds")
         for c in self.mesh.coords(axis="x"):
             c.standard_name = "grid_longitude"
         for c in self.mesh.coords(axis="y"):
             c.standard_name = "grid_latitude"
         assert self.meshcoord.standard_name == self.coord_on_mesh.standard_name
+        # Ensure updating metadata doesn't prompt the MeshCoord
+        # to update points and bounds.
+        mocked.assert_not_called()
 
     def test_updates(self, mocker):
         mocked = mocker.patch.object(MeshCoord, "_load_points_and_bounds")
@@ -1093,13 +1116,31 @@ def test_updates(self, mocker):
             np.zeros((3, 3)),
         )
         self.coord_on_mesh.points = np.zeros(3)
-        # Only the first update should actually update
-        _ = self.meshcoord.update_from_mesh()
-        _ = self.meshcoord.update_from_mesh()
+        # Only the first time you access an attribute should fetch
+        # update the points and bounds.
+
+        self.meshcoord.standard_name
+        self.meshcoord.standard_name
         mocked.assert_called_once()
 
         # Ensure it updates more than once if the mesh has been updated
-        # a second time
+        # a second time.
         self.coord_on_mesh.points = np.ones(3)
-        _ = self.meshcoord.update_from_mesh()
+        self.meshcoord.standard_name
+        assert mocked.call_count == 2
+
+    def test_update_from_mesh(self, mocker):
+        mocked = mocker.patch.object(MeshCoord, "_load_points_and_bounds")
+        mocked.return_value = (
+            np.zeros(
+                3,
+            ),
+            np.zeros((3, 3)),
+        )
+        self.coord_on_mesh.points = np.zeros(3)
+        self.meshcoord.update_from_mesh()
+        mocked.assert_called_once()
+
+        # Ensure it forces an update, even if the mesh hasn't been updated.
+        self.meshcoord.update_from_mesh()
         assert mocked.call_count == 2