From 0d9604daa90c39248b85c4a537391fdb0d1282cb Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 07:50:40 +1000 Subject: [PATCH 1/2] test: _detach is the twin of _attach, and a removed vertex leaves its graph Co-Authored-By: Claude Sonnet 5.5 --- tests/test_detach_hook.py | 133 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 tests/test_detach_hook.py diff --git a/tests/test_detach_hook.py b/tests/test_detach_hook.py new file mode 100644 index 00000000..261c58be --- /dev/null +++ b/tests/test_detach_hook.py @@ -0,0 +1,133 @@ +""" +``_attach`` records an edge in the vertices' edge lists when it is connected; +``_detach`` is its twin, undoing exactly that when the edge is removed, so each +vertex kind states how an edge enters and leaves its lists once, and +``remove_edge`` / ``remove_vertex`` need no knowledge of which kind they hold. +""" + +import unittest + +from pgraph import DGraph, DVertex, Edge, UGraph, UVertex +from pgraph.PGraph import BaseVertex + + +class TestHookIsDefinedPerKind(unittest.TestCase): + def test_kinds_supply_it(self): + self.assertIsNot(UVertex._detach, BaseVertex._detach) + self.assertIsNot(DVertex._detach, BaseVertex._detach) + self.assertIsNot(UVertex._detach, DVertex._detach) + + def test_base_hook_is_not_implemented(self): + a, b = BaseVertex(name="a"), BaseVertex(name="b") + with self.assertRaises(NotImplementedError): + a._detach(Edge(a, b), b) + + +class _Detach: + Graph: type + Vertex: type + + def setUp(self): + self.calls = [] + outer = self + + class Recording(self.Vertex): + def _detach(self, edge, dest): + # at this point the edge still knows its endpoints + outer.calls.append((self, edge, dest, edge.v1, edge.v2)) + super()._detach(edge, dest) + + self.g = self.Graph(vertex_class=Recording) + self.a = self.g.add_vertex([0, 0], name="a") + self.b = self.g.add_vertex([1, 0], name="b") + self.c = self.g.add_vertex([2, 0], name="c") + + def consistent(self): + """every edge held by a vertex is in the graph, and vice versa""" + held = {id(e) for v in self.g for e in v.edges()} + return held <= {id(e) for e in self.g.edges()} and all( + e.v1 is not None and e.v2 is not None for e in self.g.edges() + ) + + def test_remove_edge_calls_the_hook_once(self): + e = self.a.connect(self.b) + self.g.remove_edge(e) + self.assertEqual(len(self.calls), 1) + v, edge, dest, v1, v2 = self.calls[0] + self.assertIs(v, self.a) + self.assertIs(edge, e) + self.assertIs(dest, self.b) + self.assertEqual((v1, v2), (self.a, self.b)) + self.assertEqual(self.g.ne, 0) + self.assertIsNone(e.v1) + + def test_refused_removal_detaches_nothing(self): + other = self.Graph() + x, y = other.add_vertex(name="x"), other.add_vertex(name="y") + e = x.connect(y) + with self.assertRaises(ValueError): + self.g.remove_edge(e) + self.assertEqual(self.calls, []) + + def test_removing_one_of_parallel_edges(self): + e1 = self.a.connect(self.b, cost=1) + e2 = self.a.connect(self.b, cost=2) + self.g.remove_edge(e1) + self.assertEqual(self.a.edges(), [e2]) + self.assertTrue(self.consistent()) + + def test_remove_vertex_removes_its_edges_through_the_hook(self): + self.a.connect(self.b) + self.b.connect(self.c) + self.c.connect(self.a) + self.g.remove_vertex(self.b) + self.assertEqual(self.g.n, 2) + self.assertEqual(self.g.ne, 1) # c-a survives + self.assertTrue(self.consistent()) + self.assertEqual(len(self.calls), 2) # a-b and b-c + + def test_removed_vertex_no_longer_points_at_the_graph(self): + self.a.connect(self.b) + self.g.remove_vertex(self.a) + self.assertIsNone(self.a._graph) + self.assertEqual(self.a.edges(), []) + with self.assertRaises(ValueError): + self.a.remove() # not in a graph any more, a clear error + # and it can join another graph + self.Graph(vertex_class=type(self.a)).add_vertex(self.a) + self.assertEqual(self.g.n, 2) + + +class TestUndirectedDetach(_Detach, unittest.TestCase): + Graph, Vertex = UGraph, UVertex + + def test_both_ends_are_cleared(self): + e = self.a.connect(self.b) + self.g.remove_edge(e) + self.assertEqual((self.a.edges(), self.b.edges()), ([], [])) + + def test_self_loop_is_cleared_from_both_entries(self): + e = self.a.connect(self.a) + self.assertEqual(self.a.edges(), [e, e]) + self.g.remove_edge(e) + self.assertEqual(self.a.edges(), []) + + +class TestDirectedDetach(_Detach, unittest.TestCase): + Graph, Vertex = DGraph, DVertex + + def test_only_the_source_held_it(self): + e = self.a.connect(self.b) + self.g.remove_edge(e) + self.assertEqual((self.a.edges(), self.b.edges()), ([], [])) + + def test_removing_a_vertex_removes_edges_that_point_at_it(self): + self.a.connect(self.b) # a -> b, held by a only + self.g.remove_vertex(self.b) + self.assertEqual(self.a.edges(), []) + self.assertEqual(self.g.ne, 0) + self.assertTrue(self.consistent()) + + +if __name__ == "__main__": + unittest.main() From 74a87c7d4c4f891f5540e26d7e3527e4d5f9fa2c Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 07:51:14 +1000 Subject: [PATCH 2/2] refactor: vertex kinds supply _detach, the twin of _attach; removed vertices leave the graph remove_edge knew how each vertex kind held an edge (a per-endpoint 'is it in this list?' check written for the directed case). That is now the vertex kind's job: UVertex._detach removes the edge from both ends (twice for a self-loop) and DVertex._detach from the source. remove_edge calls it once, before clearing the edge's endpoints. Behaviour is unchanged. Also fixes remove_vertex leaving vertex._graph pointing at the graph it had been removed from, which made a later connect() report 'different graphs' rather than 'not in a graph', and made vertex.remove() fail confusingly. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 65 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 13 deletions(-) diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index a5f2dbf9..91852129 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -612,12 +612,9 @@ def remove_edge(self, edge: Edge) -> None: .. warning:: The connectivity of the network may be changed. - .. note:: A directed edge is tracked only by its source vertex's - edge list, not its target's (see :attr:`BaseVertex.edges`), so - membership is checked per endpoint rather than assumed for both - -- removing a directed edge from an undirected-only - implementation would otherwise raise ``ValueError`` on the - target side. + .. note:: An undirected edge is held by both of its vertices and a + directed edge only by its source (see :meth:`BaseVertex.edges`). + Each vertex kind knows which, so this method does not. .. runblock:: pycon @@ -633,15 +630,16 @@ def remove_edge(self, edge: Edge) -> None: """ if edge not in self._edgelist: raise ValueError("edge does not belong to this graph") - assert edge.v1 is not None and edge.v2 is not None + v1, v2 = edge.v1, edge.v2 + if v1 is None or v2 is None: + raise ValueError("edge does not connect two vertices") - if edge in edge.v1._edgelist: - edge.v1._edgelist.remove(edge) - if edge in edge.v2._edgelist: - edge.v2._edgelist.remove(edge) + # how the edge is held differs between undirected and directed + # vertices, so the vertex kind undoes what its _attach did + v1._detach(edge, v2) - edge.v1._connectivitychange = True - edge.v2._connectivitychange = True + v1._connectivitychange = True + v2._connectivitychange = True self._connectivitychange = True edge.v1 = None @@ -688,6 +686,8 @@ def remove_vertex(self, vertex: BaseVertex) -> None: self._vertexlist.remove(vertex) del self._vertexdict[vertex.name] + vertex._graph = None # it is not in any graph now + self._connectivitychange = True def remove(self, x: Edge | BaseVertex) -> None: """ @@ -3236,6 +3236,25 @@ def _attach(self, edge: Edge, dest: BaseVertex) -> None: "subclass UVertex or DVertex" ) + def _detach(self, edge: Edge, dest: BaseVertex) -> None: + """ + Remove an edge from the vertices' edge lists (private method) + + :param edge: the edge being removed, with ``edge.v1`` and ``edge.v2`` + still set + :param dest: the vertex at the other end, ``edge.v2`` + :raises NotImplementedError: not provided by this vertex kind + + The twin of :meth:`_attach`: it undoes exactly what that recorded. + Called by :meth:`_BaseGraph.remove_edge` before the edge's endpoints + are cleared. An edge that is absent from a list is skipped rather + than treated as an error. + """ + raise NotImplementedError( + f"{type(self).__name__} does not define how an edge is detached; " + "subclass UVertex or DVertex" + ) + def _endpoints_match(self, edge: Edge, dest: BaseVertex) -> bool: """ Test if an edge already connects this vertex to ``dest`` (private method) @@ -3571,6 +3590,17 @@ def _attach(self, edge: Edge, dest: BaseVertex) -> None: self._edgelist.append(edge) dest._edgelist.append(edge) + def _detach(self, edge: Edge, dest: BaseVertex) -> None: + """ + Remove an undirected edge from both ends (private method) + + A self-loop was recorded twice by :meth:`_attach`, so it is removed + twice. + """ + for vertex in (self, dest): + if edge in vertex._edgelist: + vertex._edgelist.remove(edge) + def _endpoints_match(self, edge: Edge, dest: BaseVertex) -> bool: """ Test if an edge already connects this vertex and ``dest`` (private method) @@ -3616,6 +3646,15 @@ def _attach(self, edge: Edge, dest: BaseVertex) -> None: """ self._edgelist.append(edge) + def _detach(self, edge: Edge, dest: BaseVertex) -> None: + """ + Remove a directed edge from its source (private method) + + ``dest`` never held it. + """ + if edge in self._edgelist: + self._edgelist.remove(edge) + # UGraph/DGraph declare their _vertex_cls here, rather than in their own # class body, because UVertex/DVertex are defined later in this file --