Merge pull request #62 from petercorke/fix/vertex-init-setattr

perf: a vertex fills its attributes directly in __init__ and add_vertex

Author: petercorke

src/pgraph/PGraph.py

@@ -1232,12 +1232,14 @@ def add_vertex(
             name = f"#{n}"
         elif name in self._vertexdict:
             raise ValueError(f"the graph already has a vertex named {name!r}")
-        vertex.name = name
+        # the vertex is in no graph yet, so the name table and the spatial index
+        # have nothing to be told; skip the property and ``__setattr__``
+        vertex.__dict__["_name"] = name
         self._vertexlist.append(vertex)
-        self._vertexdict[vertex.name] = vertex
+        self._vertexdict[name] = vertex
         if self._verbose:
-            print(f"New vertex {vertex.name}: {vertex.coord}")
-        vertex._graph = self
+            print(f"New vertex {name}: {vertex.coord}")
+        vertex.__dict__["_graph"] = self
         self._connectivitychange = True
         self._spatialchange = True
         return vertex
@@ -4466,6 +4468,14 @@ class Vertex:
     :seealso: :class:`Graph` :class:`Edge`
     """
 
+    coord: NDArray | None
+    _name: str | None
+    _label: int | None
+    _connectivitychange: bool
+    _edgelist: list[Edge]
+    _inlist: list[Edge]
+    _graph: Graph | None
+
     def __init__(self, coord: ArrayLike | None = None, name: str | None = None):
         """
         Create a vertex object
@@ -4486,20 +4496,21 @@ def __init__(self, coord: ArrayLike | None = None, name: str | None = None):
 
         :seealso: :meth:`Graph.add_vertex`
         """
-        self._edgelist: list[Edge] = []
-        if coord is None:
-            self.coord = None
-        else:
-            self.coord = np.r_[coord]
-        self._name = name
-        self._label: int | None = None
-        self._connectivitychange = True
-        self._edgelist = []
-        #: edges ending at this vertex.  Only a directed vertex fills it: an
-        #: undirected edge is already in ``_edgelist`` at both of its ends.
-        self._inlist: list[Edge] = []
-        self._graph: Graph | None = None  # reference to owning graph
-        # print('Vertex init', type(self))
+        # A vertex that is being made is in no graph, so there is no spatial
+        # index (Graph.build_kdtree) for the assignment of its coordinate to
+        # invalidate; writing the attributes directly avoids the cost of
+        # ``__setattr__`` on each of them.
+        self.__dict__.update(
+            coord=None if coord is None else np.r_[coord],
+            _name=name,
+            _label=None,
+            _connectivitychange=True,
+            _edgelist=[],  # edges leaving the vertex, or incident on it if undirected
+            # edges ending at this vertex.  Only a directed vertex fills it: an
+            # undirected edge is already in ``_edgelist`` at both of its ends.
+            _inlist=[],
+            _graph=None,  # reference to owning graph
+        )
 
     def __setattr__(self, name: str, value: Any) -> None:
         # a vertex that moves makes its graph's spatial index (build_kdtree)

tests/test_vertex_init.py

@@ -0,0 +1,134 @@
+"""
+A vertex fills its attributes directly in ``__init__``, and ``Graph.add_vertex``
+sets its name and graph directly, because the ``__setattr__`` of a vertex only
+exists to tell the graph that a coordinate moved, and a vertex that is not in
+a graph yet has no graph to tell.  These tests check that nothing else changed:
+the attributes, the name table, and that moving a vertex that is in a graph is
+still seen.
+"""
+
+import contextlib
+import io
+import unittest
+
+import numpy as np
+
+from pgraph import DGraph, UGraph, Vertex
+
+
+class TestConstructor(unittest.TestCase):
+    def test_attributes_of_a_new_vertex(self):
+        v = Vertex(coord=[1, 2], name="a")
+        np.testing.assert_array_equal(v.coord, [1, 2])
+        self.assertEqual(v.name, "a")
+        self.assertIsNone(v.label)
+        self.assertEqual(v._edgelist, [])
+        self.assertEqual(v._inlist, [])
+        self.assertIsNone(v._graph)
+        self.assertTrue(v._connectivitychange)
+
+    def test_no_coordinate(self):
+        v = Vertex()
+        self.assertIsNone(v.coord)
+        self.assertIsNone(v.name)
+
+    def test_the_coordinate_is_a_float_array_copy(self):
+        c = np.array([1.0, 2.0])
+        v = Vertex(coord=c)
+        c[0] = 99.0
+        self.assertEqual(v.coord[0], 1.0)  # not the caller's array
+        self.assertIsInstance(v.coord, np.ndarray)
+
+    def test_the_lists_are_not_shared_between_vertices(self):
+        a, b = Vertex(), Vertex()
+        self.assertIsNot(a._edgelist, b._edgelist)
+        self.assertIsNot(a._inlist, b._inlist)
+        self.assertIsNot(a._edgelist, a._inlist)
+
+    def test_subclass_with_its_own_init(self):
+        class Landmark(Vertex):
+            def __init__(self, coord=None, name=None, color=None):
+                super().__init__(coord, name)
+                self.color = color  # an attribute of its own, set after
+
+        v = Landmark([0.0, 0.0], name="l", color="red")
+        self.assertEqual((v.name, v.color), ("l", "red"))
+        g = UGraph(vertex_class=Landmark)
+        g.add_vertex(v)
+        self.assertEqual(v.color, "red")
+        self.assertIs(g["l"], v)
+
+
+class TestInAGraph(unittest.TestCase):
+    def test_name_table_and_back_pointer(self):
+        for cls in (UGraph, DGraph):
+            with self.subTest(cls=cls.__name__):
+                g = cls()
+                v = g.add_vertex([0.0, 0.0], name="v")
+                w = g.add_vertex([1.0, 0.0])  # default name
+                self.assertIs(v._graph, g)
+                self.assertIs(g["v"], v)
+                self.assertEqual(w.name, "#1")
+                self.assertIs(g["#1"], w)
+                self.assertEqual(g.validate(), [])
+
+    def test_name_from_the_vertex_is_kept(self):
+        g = UGraph()
+        v = Vertex(name="given")
+        g.add_vertex(v)
+        self.assertEqual(v.name, "given")
+        self.assertIs(g["given"], v)
+
+    def test_renaming_still_updates_the_table(self):
+        g = UGraph()
+        v = g.add_vertex(name="old")
+        v.name = "new"
+        self.assertIs(g["new"], v)
+        self.assertNotIn("old", g)
+
+    def test_duplicate_name_is_refused_and_the_vertex_stays_free(self):
+        g = UGraph()
+        g.add_vertex(name="a")
+        v = Vertex(name="b")
+        with self.assertRaises(ValueError):
+            g.add_vertex(v, name="a")
+        self.assertIsNone(v._graph)
+        self.assertEqual(v.name, "b")
+
+    def test_a_vertex_in_a_graph_cannot_be_added_to_another(self):
+        g, h = UGraph(), UGraph()
+        v = g.add_vertex()
+        with self.assertRaises(ValueError):
+            h.add_vertex(v)
+
+    def test_a_verbose_graph_says_what_it_adds(self):
+        g = UGraph(dim=2, verbose=True)
+        buf = io.StringIO()
+        with contextlib.redirect_stdout(buf):
+            g.add_vertex([1.0, 2.0], name="a")
+            g.add_vertex([3.0, 4.0])
+        lines = buf.getvalue().splitlines()
+        self.assertEqual(len(lines), 2)
+        self.assertTrue(lines[0].startswith("New vertex a: "))
+        self.assertTrue(lines[1].startswith("New vertex #1: "))
+
+    def test_assigning_a_coordinate_is_still_seen(self):
+        g = UGraph(dim=2)
+        v = g.add_vertex([0.0, 0.0])
+        g._spatialchange = False
+        v.coord = np.r_[1.0, 1.0]
+        self.assertTrue(g._spatialchange)
+        g._spatialchange = False
+        v.coord += np.r_[1.0, 0.0]
+        self.assertTrue(g._spatialchange)
+
+    def test_assigning_other_attributes_is_not_a_change(self):
+        g = UGraph(dim=2)
+        v = g.add_vertex([0.0, 0.0])
+        g._spatialchange = False
+        v.data = "anything"
+        self.assertFalse(g._spatialchange)
+
+
+if __name__ == "__main__":
+    unittest.main()