From a063fb295a30e5851bc7a99a723d48ed15b96e16 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 08:56:35 +1000 Subject: [PATCH] refactor: graph.edges() returns a list in order of addition, and both edges() methods return copies BaseGraph.edges() returned the graph's own set of edges (documented as a list), so it had no order, and it, like BaseVertex.edges(), handed out the live container: clearing the result emptied the graph's records without updating ne, and removing edges while iterating skipped some. The graph now keeps its edges in a dict used as an insertion-ordered set (keyed by the edge object, hashed by identity), so removal is still O(1), and both edges() methods return a new list. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 68 +++++++++++++++++---------- src/pgraph/exporters.py | 11 ++--- tests/test_edges_list.py | 97 +++++++++++++++++++++++++++++++++++++++ tests/test_multigraph.py | 2 +- tests/test_validate.py | 6 +-- tests/test_vertex_hook.py | 2 +- 6 files changed, 152 insertions(+), 34 deletions(-) create mode 100644 tests/test_edges_list.py diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index 61efcdf3..8727ef07 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -105,7 +105,10 @@ def __init__( # we use a list and a dict, the list respects the order of adding self._vertexlist: list[BaseVertex] = [] self._vertexdict: dict[str, BaseVertex] = {} - self._edgelist: set[Edge] = set() + # an insertion-ordered set: the keys are the edges, hashed by identity + # (Edge defines no __eq__), the values are unused. This gives O(1) + # removal and a defined order. + self._edges: dict[Edge, None] = {} self._verbose = verbose self._dim = dim self._ncomponents = 0 @@ -377,11 +380,11 @@ def validate(self) -> list[str]: else: problems.append(f"the vertex list holds {entry!r}, which is not a vertex") graph_edges: list[Edge] = [] - for member in self._edgelist: + for member in self._edges: if isinstance(member, Edge): graph_edges.append(member) else: - problems.append(f"the edge set holds {member!r}, which is not an edge") + problems.append(f"the graph's edges hold {member!r}, which is not an edge") # ---- vertices members = {id(v) for v in vertices} @@ -844,7 +847,7 @@ def remove_edge(self, edge: Edge) -> None: :seealso: :meth:`remove_vertex` :meth:`Edge.remove` """ - if edge not in self._edgelist: + if edge not in self._edges: raise ValueError("edge does not belong to this graph") v1, v2 = edge.v1, edge.v2 if v1 is None or v2 is None: @@ -860,7 +863,7 @@ def remove_edge(self, edge: Edge) -> None: edge._set_endpoints(None, None) - self._edgelist.remove(edge) + del self._edges[edge] def remove_vertex(self, vertex: BaseVertex) -> None: """ @@ -951,7 +954,7 @@ def show(self) -> None: for v in self._vertexlist: print(" " + str(v)) print("edges:") - for e in self._edgelist: + for e in self._edges: print(" " + str(e)) @property @@ -989,7 +992,7 @@ def ne(self) -> int: >>> g.add_edge(v1, v2) >>> print(g.ne) """ - return len(self._edgelist) + return len(self._edges) @abstractmethod def _graphcolor(self) -> int | None: @@ -1327,21 +1330,35 @@ def closest(self, coord: ArrayLike) -> tuple[BaseVertex | None, float]: return min_which, min_dist - def edges(self) -> set[Edge]: + def edges(self) -> list[Edge]: """ Get all edges in graph (base class method) :return: All edges in the graph - :rtype: set of Edge references + :rtype: list of Edge references + + The edges are in the order they were added to the graph. This is a + new list, so it can be modified or sorted without affecting the graph, + and edges can be removed from the graph while iterating over it. We can iterate over all edges in the graph by:: for e in g.edges(): print(e) - .. note:: Unlike :meth:`BaseVertex.edges`, which returns a ``list`` in - connection order, this returns a ``set`` with no defined - iteration order. + and select one by index, for example the second edge added is + ``g.edges()[1]``. + + .. note:: Like :meth:`BaseVertex.edges` this is a ``list``, but where + that lists the edges attached to one vertex, this lists every edge + in the graph once. A self-loop is one edge here, but appears + twice in the :meth:`BaseVertex.edges` of an undirected vertex + because it has two ends there. + + .. versionchanged:: 1.0 + Returns a ``list`` in order of addition. In 0.6.5 this was the + graph's own ``set``, with no defined order, although the + documentation said ``list``. .. runblock:: pycon @@ -1352,9 +1369,9 @@ def edges(self) -> set[Edge]: >>> g.add_edge(v1, v2) >>> print(g.edges()) - :seealso: :meth:`BaseVertex.edges` + :seealso: :meth:`BaseVertex.edges` :attr:`ne` """ - return self._edgelist + return list(self._edges) def plot( self, @@ -2224,14 +2241,14 @@ def incidence(self) -> NDArray: .. note:: - vertices are numbered in their order of creation. A vertex index can be resolved to a vertex reference by ``graph[i]``. - - edges are numbered in the order they appear in ``graph.edges()``. + - edges are numbered in the order they appear in :meth:`edges`, which + is the order they were added. - Both endpoints of every edge are marked, regardless of direction -- for a ``DGraph`` this means a vertex that is only - ever a target (never a source) still appears here, unlike - :meth:`degree`/:attr:`BaseVertex.degree`, which count outgoing - edges only. Iterating each vertex's own - :meth:`BaseVertex.edges` instead would silently drop such - vertices for a directed graph. + ever a target (never a source) still appears here. Iterating + each vertex's own :meth:`BaseVertex.edges`, which are its + outgoing edges, would silently drop such vertices for a + directed graph. .. rubric:: For graphs with parallel edges or loops @@ -3541,7 +3558,7 @@ def connect( else: e = self._graph._edge_class(self, dest, cost=cost, data=data) - self._graph._edgelist.add(e) + self._graph._edges[e] = None self._graph._connectivitychange = True self._connectivitychange = True @@ -3661,11 +3678,16 @@ def edges(self) -> list[Edge]: :return: List of all edges leaving this vertex :rtype: list of Edge + The edges are in the order they were connected. This is a new list, + so it can be modified, and edges can be removed from the graph while + iterating over it. + .. note:: - For a directed graph the edges are those leaving this vertex - For a non-directed graph the edges are those leaving or entering - this vertex + this vertex, and a self-loop is listed twice, once for each of + its ends .. runblock:: pycon @@ -3679,7 +3701,7 @@ def edges(self) -> list[Edge]: :seealso: :meth:`out_edges` :meth:`in_edges` :meth:`adjacent` :meth:`incidences` :attr:`degree` :meth:`BaseGraph.edges` """ - return self._edgelist + return list(self._edgelist) def heuristic_distance(self, v2: BaseVertex) -> float: """ diff --git a/src/pgraph/exporters.py b/src/pgraph/exporters.py index 52ebc435..c987c9d9 100644 --- a/src/pgraph/exporters.py +++ b/src/pgraph/exporters.py @@ -141,12 +141,11 @@ def _ordered_edges(graph: BaseGraph) -> list[tuple[Edge, BaseVertex, BaseVertex] """ Edges in a reproducible order - ``graph.edges()`` is a set, so its order changes from run to run and the - exported text would not be stable (useless for diffs and golden files). - Instead walk the vertices in creation order and each vertex's own - outgoing edges, keeping the first sight of every edge. Identity is by object, not by - endpoints, so parallel edges are all kept, and an undirected edge (held by - both ends, twice for a self-loop) is listed once. + Walk the vertices in creation order and each vertex's own outgoing edges, + keeping the first sight of every edge, so that edges are grouped by the + vertex they leave. Identity is by object, not by endpoints, so parallel + edges are all kept, and an undirected edge (held by both ends, twice for a + self-loop) is listed once. Each item is ``(edge, start, end)``. """ diff --git a/tests/test_edges_list.py b/tests/test_edges_list.py new file mode 100644 index 00000000..52cdc1f9 --- /dev/null +++ b/tests/test_edges_list.py @@ -0,0 +1,97 @@ +"""``graph.edges()`` and ``vertex.edges()`` return new lists in order of addition. + +In 0.6.5 ``graph.edges()`` returned the graph's own ``set`` (documented as a +list), so it had no order, and either method returned the live container, so +a caller could corrupt the graph by modifying the result. +""" + +import unittest + +from pgraph import DGraph, UGraph + + +class _EdgesList: + cls: type + + def setUp(self): + self.g = self.cls() + self.a, self.b, self.c = (self.g.add_vertex() for _ in range(3)) + self.e1 = self.g.add_edge(self.a, self.b) + self.e2 = self.g.add_edge(self.b, self.c) + self.e3 = self.g.add_edge(self.a, self.c) + + def test_is_a_list_in_order_of_addition(self): + edges = self.g.edges() + self.assertIsInstance(edges, list) + self.assertEqual(edges, [self.e1, self.e2, self.e3]) + + def test_order_survives_removal(self): + self.g.remove_edge(self.e2) + e4 = self.g.add_edge(self.b, self.c) + self.assertEqual(self.g.edges(), [self.e1, self.e3, e4]) + + def test_graph_result_is_a_copy(self): + self.g.edges().clear() + self.assertEqual(self.g.ne, 3) + self.assertEqual(self.g.edges(), [self.e1, self.e2, self.e3]) + self.assertEqual(self.g.validate(), []) + + def test_vertex_result_is_a_copy(self): + self.a.edges().clear() + self.assertEqual(len(self.a.edges()), 2) + self.assertEqual(self.g.validate(), []) + + def test_remove_while_iterating_visits_every_edge(self): + seen = [] + for e in self.g.edges(): + seen.append(e) + self.g.remove_edge(e) + self.assertEqual(seen, [self.e1, self.e2, self.e3]) + self.assertEqual(self.g.ne, 0) + + def test_vertex_remove_while_iterating_visits_every_edge(self): + seen = [] + for e in self.a.edges(): + seen.append(e) + self.g.remove_edge(e) + self.assertEqual(seen, [self.e1, self.e3]) + self.assertEqual(self.a.edges(), []) + + def test_parallel_edges_are_both_listed(self): + e4 = self.g.add_edge(self.a, self.b) + self.assertEqual(self.g.edges().count(e4), 1) + self.assertEqual(len(self.g.edges()), 4) + self.assertEqual(self.g.ne, 4) + + def test_loop_is_one_graph_edge(self): + loop = self.g.add_edge(self.c, self.c) + self.assertEqual(self.g.edges().count(loop), 1) + self.assertEqual(self.g.ne, 4) + + def test_edge_removed_via_edge_leaves_the_list(self): + self.e1.remove() + self.assertEqual(self.g.edges(), [self.e2, self.e3]) + + def test_vertex_removal_removes_its_edges(self): + self.g.remove_vertex(self.b) + self.assertEqual(self.g.edges(), [self.e3]) + + +class TestUGraphEdgesList(_EdgesList, unittest.TestCase): + cls = UGraph + + def test_loop_is_listed_twice_at_its_vertex(self): + loop = self.g.add_edge(self.c, self.c) + self.assertEqual(self.c.edges().count(loop), 2) + + +class TestDGraphEdgesList(_EdgesList, unittest.TestCase): + cls = DGraph + + def test_loop_is_listed_once_at_its_vertex(self): + loop = self.g.add_edge(self.c, self.c) + self.assertEqual(self.c.edges().count(loop), 1) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_multigraph.py b/tests/test_multigraph.py index 9199dec7..3e61a8a2 100644 --- a/tests/test_multigraph.py +++ b/tests/test_multigraph.py @@ -1,7 +1,7 @@ """ Self-loops and parallel edges. -pgraph does not forbid either, so every quantity derived from the edge set +pgraph does not forbid either, so every quantity derived from the graph's edges (degree, the matrix representations, cycle detection, path planning) has to give a defined, consistent answer when they are present. diff --git a/tests/test_validate.py b/tests/test_validate.py index 7c721e6f..62392827 100644 --- a/tests/test_validate.py +++ b/tests/test_validate.py @@ -178,7 +178,7 @@ def test_junk_in_the_containers_is_reported_not_raised(self): # validate() is for finding corruption, so it must survive it g, a, b, c, d = build(UGraph) g._vertexlist.append("junk") - g._edgelist.add(42) + g._edges[42] = None a._edgelist.append("junk") msg = self.problems(g) for word in ("junk", "42", "not a vertex", "not an edge"): @@ -209,14 +209,14 @@ class MyEdge(Edge): g = UGraph(edge_class=MyEdge) a, b = g.add_vertex(name="a"), g.add_vertex(name="b") e = Edge(a, b) - g._edgelist.add(e) + g._edges[e] = None a._edgelist.append(e) b._edgelist.append(e) self.assertIn("MyEdge", self.problems(g)) def test_vertex_list_entry_for_an_edge_removed_from_the_graph(self): g, a, b, c, d = build(UGraph) - g._edgelist.discard(a.edges()[0]) + g._edges.pop(a.edges()[0], None) self.assertIn("not in the graph", self.problems(g)) def test_self_loop_held_once_instead_of_twice(self): diff --git a/tests/test_vertex_hook.py b/tests/test_vertex_hook.py index 771bb34a..4720ce22 100644 --- a/tests/test_vertex_hook.py +++ b/tests/test_vertex_hook.py @@ -59,7 +59,7 @@ class Recording(self.Vertex): def _attach(self, edge, dest): # the edge is already registered with the graph at this point outer.calls.append( - (self, edge, dest, edge in self._graph._edgelist) + (self, edge, dest, edge in self._graph._edges) ) super()._attach(edge, dest)