From d134581bafc4f86f1e693ae4312c0d6964348362 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 07:52:20 +1000 Subject: [PATCH 1/7] test: directed vertices know their incoming edges; degree is in plus out Co-Authored-By: Claude Sonnet 5.5 --- tests/test_in_edges.py | 203 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 tests/test_in_edges.py diff --git a/tests/test_in_edges.py b/tests/test_in_edges.py new file mode 100644 index 00000000..08395b95 --- /dev/null +++ b/tests/test_in_edges.py @@ -0,0 +1,203 @@ +""" +A directed vertex knows its incoming edges as well as its outgoing ones. + +That gives ``predecessors()`` (to go with ``successors()``, which is +``adjacent()``), ``in_degree`` and ``out_degree``, and makes ``degree`` the +graph-theory degree: in plus out, so a self-loop counts 2 and the degrees of a +graph sum to twice its edges. For an undirected graph in, out and total +degree are all the same. +""" + +import random +import unittest + +import numpy as np + +from pgraph import DGraph, UGraph + + +def sample_digraph(): + """ + a -> b (x3, parallel) b -> c c -> a d -> d (loop) + """ + g = DGraph() + a, b, c, d = (g.add_vertex([i, 0], name=n) for i, n in enumerate("abcd")) + for _ in range(3): + a.connect(b) + b.connect(c) + c.connect(a) + d.connect(d) + return g, a, b, c, d + + +class TestDirectedDegrees(unittest.TestCase): + def setUp(self): + self.g, self.a, self.b, self.c, self.d = sample_digraph() + + def test_out_degree(self): + self.assertEqual([v.out_degree for v in self.g], [3, 1, 1, 1]) + + def test_in_degree(self): + self.assertEqual([v.in_degree for v in self.g], [1, 3, 1, 1]) + + def test_degree_is_in_plus_out(self): + self.assertEqual([v.degree for v in self.g], [4, 4, 2, 2]) + for v in self.g: + self.assertEqual(v.degree, v.in_degree + v.out_degree) + + def test_a_self_loop_counts_two(self): + self.assertEqual(self.d.in_degree, 1) + self.assertEqual(self.d.out_degree, 1) + self.assertEqual(self.d.degree, 2) + + def test_handshake(self): + # every edge has two ends + self.assertEqual(sum(v.degree for v in self.g), 2 * self.g.ne) + self.assertEqual(sum(v.in_degree for v in self.g), self.g.ne) + self.assertEqual(sum(v.out_degree for v in self.g), self.g.ne) + + def test_graph_degree_matrix_is_the_total(self): + np.testing.assert_array_equal(np.diag(self.g.degree()), [4, 4, 2, 2]) + self.assertEqual(self.g.connectivity(), [4, 4, 2, 2]) + + def test_average_degree(self): + self.assertAlmostEqual(self.g.average_degree(), 2 * self.g.ne / self.g.n) + + def test_laplacian_still_has_zero_row_sums(self): + # a directed Laplacian uses the OUT-degree: D_out - A + L = self.g.Laplacian() + np.testing.assert_allclose(L.sum(axis=1), 0) + np.testing.assert_array_equal( + L, np.diag([v.out_degree for v in self.g]) - self.g.adjacency() + ) + + def test_edges_is_still_outgoing(self): + self.assertEqual(len(self.a.edges()), 3) + self.assertEqual(len(self.b.edges()), 1) + + +class TestPredecessorsSuccessors(unittest.TestCase): + def setUp(self): + self.g, self.a, self.b, self.c, self.d = sample_digraph() + + def names(self, vs): + return sorted(v.name for v in vs) + + def test_successors_are_the_adjacent_vertices(self): + self.assertEqual(self.a.successors(), self.a.adjacent()) + self.assertEqual(self.names(self.a.successors()), ["b", "b", "b"]) + + def test_predecessors(self): + self.assertEqual(self.names(self.b.predecessors()), ["a", "a", "a"]) + self.assertEqual(self.names(self.a.predecessors()), ["c"]) + self.assertEqual(self.names(self.c.predecessors()), ["b"]) + + def test_loop_is_its_own_predecessor_and_successor(self): + self.assertEqual(self.d.predecessors(), [self.d]) + self.assertEqual(self.d.successors(), [self.d]) + + def test_in_edges_and_out_edges(self): + self.assertEqual(len(self.b.in_edges()), 3) + self.assertTrue(all(e.v2 is self.b for e in self.b.in_edges())) + self.assertEqual(len(self.a.out_edges()), 3) + self.assertTrue(all(e.v1 is self.a for e in self.a.out_edges())) + + def test_results_are_copies(self): + self.b.predecessors().clear() + self.b.in_edges().clear() + self.assertEqual(self.b.in_degree, 3) + + def test_predecessor_of_a_source_is_empty(self): + g = DGraph() + a, b = g.add_vertex(name="a"), g.add_vertex(name="b") + a.connect(b) + self.assertEqual(a.predecessors(), []) + self.assertEqual(b.successors(), []) + + +class TestUndirected(unittest.TestCase): + def setUp(self): + self.g = UGraph() + self.a, self.b, self.c = (self.g.add_vertex(name=n) for n in "abc") + self.a.connect(self.b) + self.a.connect(self.b) + self.b.connect(self.c) + self.c.connect(self.c) + + def test_in_out_and_total_agree(self): + for v in self.g: + self.assertEqual(v.in_degree, v.degree) + self.assertEqual(v.out_degree, v.degree) + self.assertEqual([v.degree for v in self.g], [2, 3, 3]) # loop counts 2 + + def test_predecessors_and_successors_are_adjacent(self): + for v in self.g: + self.assertEqual(v.predecessors(), v.adjacent()) + self.assertEqual(v.successors(), v.adjacent()) + self.assertEqual(len(self.b.in_edges()), 3) + self.assertEqual(len(self.b.out_edges()), 3) + + def test_handshake(self): + self.assertEqual(sum(v.degree for v in self.g), 2 * self.g.ne) + + +class TestInEdgeListStaysConsistent(unittest.TestCase): + """the in-list is a second record of every edge; it must never drift""" + + def check(self, g): + out_ids = {id(e) for v in g for e in v._edgelist} + in_ids = {id(e) for v in g for e in v._inlist} + graph_ids = {id(e) for e in g.edges()} + self.assertEqual(out_ids, graph_ids) + self.assertEqual(in_ids, graph_ids) + for v in g: + self.assertTrue(all(e.v1 is v for e in v._edgelist)) + self.assertTrue(all(e.v2 is v for e in v._inlist)) + self.assertEqual(sum(len(v._inlist) for v in g), g.ne) + self.assertEqual(sum(len(v._edgelist) for v in g), g.ne) + + def test_after_connect_and_remove(self): + g, a, b, c, d = sample_digraph() + self.check(g) + g.remove_edge(a.edges()[0]) + self.check(g) + g.remove_vertex(b) + self.check(g) + self.assertEqual(c.predecessors(), []) # b is gone + self.assertEqual(a.successors(), []) + + def test_removing_a_vertex_clears_edges_in_both_directions(self): + g, a, b, c, d = sample_digraph() + g.remove_vertex(a) # has 3 out, 1 in + self.assertEqual(g.ne, 2) # b->c and d->d remain + self.check(g) + self.assertEqual(b.in_degree, 0) + self.assertEqual(c.out_degree, 0) + + def test_random_sequences(self): + rng = random.Random(7) + for trial in range(25): + g = DGraph() + vs = [g.add_vertex([i, 0]) for i in range(8)] + for _ in range(rng.randrange(0, 40)): + op = rng.random() + if op < 0.65 or g.ne == 0: + rng.choice(vs).connect(rng.choice(vs)) + elif op < 0.9: + g.remove_edge(rng.choice(sorted(g.edges(), key=id))) + elif g.n > 2: + v = rng.choice(list(g)) + g.remove_vertex(v) + vs = [x for x in vs if x is not v] + self.check(g) + self.assertEqual(sum(v.degree for v in g), 2 * g.ne) + + def test_deepcopy_keeps_it_consistent(self): + g, *_ = sample_digraph() + h = g.copy() + self.check(h) + self.assertIsNot(h[0]._inlist, g[0]._inlist) + + +if __name__ == "__main__": + unittest.main() From 4025b100d6acaa18f9260fad2f3dbc461ead4304 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 08:22:03 +1000 Subject: [PATCH 2/7] refactor: BaseGraph is the public name of the common graph base class The class was named _BaseGraph when PGraph was split into a base class and UGraph/DGraph, but it is in the signature of every graph method, is shown in the documentation, and is what a user writes in a type hint, while its counterpart BaseVertex has no underscore. _BaseGraph was never released, so there is no alias. BaseGraph and BaseVertex are exported from pgraph, with an __all__. Mechanical: with the rename reversed, PGraph.py and exporters.py are byte-identical to the previous commit. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 66 ++++++++++++++++++++--------------------- src/pgraph/__init__.py | 20 ++++++++++++- src/pgraph/exporters.py | 20 ++++++------- 3 files changed, 62 insertions(+), 44 deletions(-) diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index 308b7965..5b8b12b9 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -16,7 +16,7 @@ from spatialmath.base.graphics import axes_logic -class _BaseGraph(ABC): +class BaseGraph(ABC): #: concrete vertex type for this graph kind, provided by :class:`UGraph` #: and :class:`DGraph` -- lets :meth:`add_vertex` and :meth:`vertex_copy` @@ -140,7 +140,7 @@ def __repr__(self) -> str: return str(self) @classmethod - def Dict(cls, d: dict, reverse: bool = False, **kwargs: Any) -> _BaseGraph: + def Dict(cls, d: dict, reverse: bool = False, **kwargs: Any) -> BaseGraph: """ Create graph from parent/child dictionary @@ -150,7 +150,7 @@ def Dict(cls, d: dict, reverse: bool = False, **kwargs: Any) -> _BaseGraph: :param kwargs: optional arguments passed to the graph constructor, for example ``vertex_class`` or ``edge_class`` :raises TypeError: ``vertex_class`` or ``edge_class`` is not a - suitable class, see :meth:`_BaseGraph.__init__` + suitable class, see :meth:`BaseGraph.__init__` :return: graph :rtype: UGraph or DGraph @@ -207,7 +207,7 @@ def Adjacency( coords: NDArray | None = None, names: list[str] | None = None, **kwargs: Any, - ) -> _BaseGraph: + ) -> BaseGraph: """ Create graph from adjacency matrix @@ -222,7 +222,7 @@ def Adjacency( :raises ValueError: ``A`` is not square, or the length of ``names`` or the number of rows of ``coords`` does not match ``A`` :raises TypeError: ``vertex_class`` or ``edge_class`` is not a - suitable class, see :meth:`_BaseGraph.__init__` + suitable class, see :meth:`BaseGraph.__init__` :return: graph :rtype: UGraph or DGraph @@ -306,12 +306,12 @@ def edge_class(self) -> type[Edge]: """ return self._edge_class - def copy(self) -> _BaseGraph: + def copy(self) -> BaseGraph: """ Deepcopy of graph :return: deep copy - :rtype: _BaseGraph + :rtype: BaseGraph .. runblock:: pycon @@ -370,7 +370,7 @@ def add_vertex( >>> print(v2.name) If the graph was constructed with a required ``dim`` (see - :meth:`_BaseGraph.__init__`), every embedded vertex must have a + :meth:`BaseGraph.__init__`), every embedded vertex must have a coordinate of exactly that length -- adding one of the wrong length raises ``ValueError``: @@ -2478,18 +2478,18 @@ def path_Astar( # -------------------------------------------------------------------------- # -class UGraph(_BaseGraph): +class UGraph(BaseGraph): """ Class for undirected graphs Vertices are :class:`UVertex` objects, or instances of the subclass given as ``vertex_class``. Edges are :class:`Edge` objects, or instances of the - subclass given as ``edge_class``; see :class:`_BaseGraph` for the + subclass given as ``edge_class``; see :class:`BaseGraph` for the constructor arguments and the exceptions they raise. .. inheritance-diagram:: UGraph - :seealso: :class:`_BaseGraph` :class:`DGraph` + :seealso: :class:`BaseGraph` :class:`DGraph` """ def _graphcolor(self) -> None: @@ -2537,18 +2537,18 @@ def _graphcolor(self) -> None: self._ncomponents = 0 if lastlabel is None else lastlabel + 1 -class DGraph(_BaseGraph): +class DGraph(BaseGraph): """ Class for directed graphs Vertices are :class:`DVertex` objects, or instances of the subclass given as ``vertex_class``. Edges are :class:`Edge` objects, or instances of the - subclass given as ``edge_class``; see :class:`_BaseGraph` for the + subclass given as ``edge_class``; see :class:`BaseGraph` for the constructor arguments and the exceptions they raise. .. inheritance-diagram:: DGraph - :seealso: :class:`_BaseGraph` :class:`UGraph` + :seealso: :class:`BaseGraph` :class:`UGraph` """ def _graphcolor(self) -> int | None: @@ -2654,7 +2654,7 @@ class Edge: - Inheritance is an alternative to providing arbitrary user data. - Pass the subclass as ``edge_class`` when constructing the graph and the graph will create instances of it itself, see - :class:`_BaseGraph`. + :class:`BaseGraph`. An Edge points to a pair of vertices. At ``connect`` time the vertices get references back to the Edge object. @@ -2828,7 +2828,7 @@ def connect(self, v1: BaseVertex, v2: BaseVertex) -> None: :raises ValueError: either vertex has not been added to a graph, or the vertices belong to different graphs :raises TypeError: this edge is not an instance of the graph's - :attr:`_BaseGraph.edge_class` + :attr:`BaseGraph.edge_class` The edge connects vertices ``v1`` and ``v2``, and is added to the graph that those vertices belong to. @@ -2948,13 +2948,13 @@ def remove(self) -> None: ``e.remove()`` removes ``e`` from its graph's own edge collection and from its connected vertices' edge lists (both, for an undirected edge; the source only, for a directed one -- see - :meth:`_BaseGraph.remove_edge`), and clears ``e.v1``/``e.v2`` to + :meth:`BaseGraph.remove_edge`), and clears ``e.v1``/``e.v2`` to ``None``. The ``Edge`` object itself is not deleted -- it becomes an orphaned, disconnected shell, and calling ``remove()`` on it again raises ``ValueError``. .. note:: This is a thin convenience wrapper around - :meth:`_BaseGraph.remove_edge`. + :meth:`BaseGraph.remove_edge`. :rtype: None @@ -2968,7 +2968,7 @@ def remove(self) -> None: >>> e.remove() >>> print(g) - :seealso: :meth:`_BaseGraph.remove_edge` :meth:`BaseVertex.remove` + :seealso: :meth:`BaseGraph.remove_edge` :meth:`BaseVertex.remove` """ if self.v1 is None or self.v1._graph is None: raise ValueError("edge is not connected to a graph") @@ -3001,7 +3001,7 @@ def __init__(self, coord: ArrayLike | None = None, name: str | None = None): :type name: str, optional Creates a vertex but does not add it to a graph -- use - :meth:`_BaseGraph.add_vertex` for that. + :meth:`BaseGraph.add_vertex` for that. .. runblock:: pycon @@ -3009,7 +3009,7 @@ def __init__(self, coord: ArrayLike | None = None, name: str | None = None): >>> v1 = UVertex(coord=[0,0], name='v1') >>> print(v1) - :seealso: :meth:`_BaseGraph.add_vertex` + :seealso: :meth:`BaseGraph.add_vertex` """ self._edgelist: list[Edge] = [] if coord is None: @@ -3020,7 +3020,7 @@ def __init__(self, coord: ArrayLike | None = None, name: str | None = None): self.label: int | None = None self._connectivitychange = True self._edgelist = [] - self._graph: _BaseGraph | None = None # reference to owning graph + self._graph: BaseGraph | None = None # reference to owning graph # print('BaseVertex init', type(self)) def __str__(self) -> str: @@ -3057,7 +3057,7 @@ def __repr__(self) -> str: coord = ", ".join([f"{x:.4g}" for x in self.coord]) return f"{self.__class__.__name__}[{self.name:s}, coord=({coord})]" - def copy(self, cls: type[_BaseGraph] | None = None) -> BaseVertex: + def copy(self, cls: type[BaseGraph] | None = None) -> BaseVertex: """ Copy a vertex @@ -3094,7 +3094,7 @@ def adjacent(self) -> list[BaseVertex]: ``v.adjacent()`` is a list of the vertices that this vertex is joined to by an edge. "Adjacent" is the graph-theory term, and it is the - relation that :meth:`_BaseGraph.adjacency` tabulates: row ``i`` of the + relation that :meth:`BaseGraph.adjacency` tabulates: row ``i`` of the adjacency matrix is non-zero exactly at the columns of the vertices returned by ``g[i].adjacent()``. @@ -3120,7 +3120,7 @@ def adjacent(self) -> list[BaseVertex]: >>> print(v1.adjacent()) :seealso: :meth:`isadjacent` :meth:`incidences` :meth:`edges` - :meth:`_BaseGraph.adjacency` + :meth:`BaseGraph.adjacency` """ return [e.next(self) for e in self._edgelist] @@ -3198,7 +3198,7 @@ def connect( :param dest: The vertex to connect to :type dest: ``BaseVertex`` subclass :param edge: Use this as the edge object, otherwise a new object of the - graph's :attr:`_BaseGraph.edge_class` is created from the + graph's :attr:`BaseGraph.edge_class` is created from the vertices being connected, and the ``cost`` and ``data`` parameters, defaults to None :type edge: ``Edge`` subclass, optional @@ -3211,7 +3211,7 @@ def connect( vertices belong to different graphs, or ``edge`` already connects other vertices :raises TypeError: ``edge`` is not an instance of the graph's - :attr:`_BaseGraph.edge_class` + :attr:`BaseGraph.edge_class` :return: the edge connecting the vertices :rtype: Edge @@ -3302,7 +3302,7 @@ def _detach(self, edge: Edge, dest: BaseVertex) -> None: :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 + 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. """ @@ -3409,7 +3409,7 @@ def heuristic_distance(self, v2: BaseVertex) -> float: :rtype: float Distance is computed according to the graph's heuristic, see - :meth:`_BaseGraph.heuristic`. The vertex must belong to a graph, + :meth:`BaseGraph.heuristic`. The vertex must belong to a graph, since the heuristic is a property of the graph, not the vertex. .. runblock:: pycon @@ -3420,7 +3420,7 @@ def heuristic_distance(self, v2: BaseVertex) -> float: >>> v2 = g.add_vertex(coord=[4, 3], name='v2') >>> print(v1.heuristic_distance(v2)) - :seealso: :meth:`distance` :meth:`_BaseGraph.heuristic` + :seealso: :meth:`distance` :meth:`BaseGraph.heuristic` """ if self._graph is None: raise ValueError("vertex is not connected to a graph") @@ -3569,7 +3569,7 @@ def closest(self) -> tuple[BaseVertex | None, float]: >>> v4 = g.add_vertex(coord=[4,3], name='v4') >>> print(v1.closest()) - :seealso: :meth:`_BaseGraph.closest` + :seealso: :meth:`BaseGraph.closest` """ if self._graph is None: raise ValueError("vertex is not connected to a graph") @@ -3588,7 +3588,7 @@ def remove(self) -> None: not deleted. .. note:: This is a thin convenience wrapper around - :meth:`_BaseGraph.remove_vertex`. + :meth:`BaseGraph.remove_vertex`. :rtype: None @@ -3602,7 +3602,7 @@ def remove(self) -> None: >>> v2.remove() >>> print(g) - :seealso: :meth:`_BaseGraph.remove_vertex` :meth:`Edge.remove` + :seealso: :meth:`BaseGraph.remove_vertex` :meth:`Edge.remove` """ if self._graph is None: raise ValueError("vertex is not connected to a graph") diff --git a/src/pgraph/__init__.py b/src/pgraph/__init__.py index 011d41c7..a823ff58 100644 --- a/src/pgraph/__init__.py +++ b/src/pgraph/__init__.py @@ -1 +1,19 @@ -from pgraph.PGraph import UGraph, DGraph, UVertex, DVertex, Edge +from pgraph.PGraph import ( + BaseGraph, + BaseVertex, + UGraph, + DGraph, + UVertex, + DVertex, + Edge, +) + +__all__ = [ + "BaseGraph", + "BaseVertex", + "UGraph", + "DGraph", + "UVertex", + "DVertex", + "Edge", +] diff --git a/src/pgraph/exporters.py b/src/pgraph/exporters.py index 0ea70e95..5cc28744 100644 --- a/src/pgraph/exporters.py +++ b/src/pgraph/exporters.py @@ -2,7 +2,7 @@ Export a graph as text (GraphViz DOT, Mermaid, GraphML, ELK JSON) or as a rendered image (via the GraphViz command line tools). -This module is reached through :meth:`pgraph.PGraph._BaseGraph.export`, which +This module is reached through :meth:`pgraph.PGraph.BaseGraph.export`, which dispatches on the format name or the file extension. Each text exporter is a plain function ``f(graph, **options) -> str``, registered with :func:`register_exporter`, so a user can add a format without touching the @@ -27,7 +27,7 @@ def gexf(graph, *, version="1.3") -> str: from typing import IO, TYPE_CHECKING, Any, Callable if TYPE_CHECKING: - from .PGraph import BaseVertex, Edge, _BaseGraph + from .PGraph import BaseVertex, Edge, BaseGraph TextExporter = Callable[..., str] @@ -114,13 +114,13 @@ def format_from_filename(filename: str | os.PathLike | IO | None) -> str | None: # helpers shared by the exporters -def _is_directed(graph: _BaseGraph) -> bool: +def _is_directed(graph: BaseGraph) -> bool: from .PGraph import DGraph # not at module level: PGraph imports this module return isinstance(graph, DGraph) -def _ordered_edges(graph: _BaseGraph) -> list[tuple[Edge, BaseVertex, BaseVertex]]: +def _ordered_edges(graph: BaseGraph) -> list[tuple[Edge, BaseVertex, BaseVertex]]: """ Edges in a reproducible order @@ -144,7 +144,7 @@ def _ordered_edges(graph: _BaseGraph) -> list[tuple[Edge, BaseVertex, BaseVertex return edges -def _component_colors(graph: _BaseGraph) -> dict[BaseVertex, str]: +def _component_colors(graph: BaseGraph) -> dict[BaseVertex, str]: """ Map each vertex to a ``#rrggbb`` colour for its component, as plot() does """ @@ -194,7 +194,7 @@ def _dot_quote(text: str) -> str: @register_exporter("dot", ".dot", ".gv") def dot( - graph: _BaseGraph, + graph: BaseGraph, *, direction: str | None = None, colorcomponents: bool = False, @@ -243,7 +243,7 @@ def dot( @register_exporter("mermaid", ".mmd", ".mermaid") def mermaid( - graph: _BaseGraph, + graph: BaseGraph, *, direction: str = "TB", colorcomponents: bool = False, @@ -292,7 +292,7 @@ def mermaid( @register_exporter("graphml", ".graphml") -def graphml(graph: _BaseGraph) -> str: +def graphml(graph: BaseGraph) -> str: """ GraphML XML text @@ -357,7 +357,7 @@ def key(key_id: str, domain: str, name: str, type_: str) -> str: @register_exporter("elk", ".elk.json", ".elk") def elk( - graph: _BaseGraph, + graph: BaseGraph, *, size: tuple[float, float] = (60, 40), layout_options: dict[str, str] | None = None, @@ -433,7 +433,7 @@ def elk( def render( - graph: _BaseGraph, + graph: BaseGraph, fmt: str, *, engine: str | None = None, From b588fbe0924c34f4c7242cae5421888c5407e656 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 08:26:52 +1000 Subject: [PATCH 3/7] feat: directed vertices record their incoming edges; degree is in plus out A directed vertex now holds the edges that end at it as well as the ones that leave it, kept in step by _attach/_detach. That gives: - predecessors(), successors() (which is adjacent()), in_edges(), out_edges() - in_degree and out_degree - remove_vertex finding a vertex's edges from its own lists rather than scanning every edge in the graph (O(degree), was O(E)) - iscyclic taking in-degrees from the vertices (O(V), was O(E)) BREAKING, for a DGraph: degree is now the graph-theory degree, in plus out, so a self-loop counts 2 and the degrees of a graph sum to twice its edges. It counted only the edges leaving the vertex. This changes: vertex.degree (use out_degree for the old value) graph.degree() (np.diag([v.out_degree for v in g])) graph.connectivity() ([v.out_degree for v in g]) graph.average_degree() 2E/N, was E/N (g.ne / g.n) Laplacian() is unchanged: it is D_out - A, and now takes the out-degree explicitly instead of relying on degree(). For an undirected graph every quantity is as before. Each is documented with a versionchanged note, and a survey of RTB, MVTB and RVC3 found no caller of any of them on a DGraph. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 292 ++++++++++++++++++++++++++++++-------- tests/test_graph.py | 16 ++- tests/test_multigraph.py | 19 ++- tests/test_vertex_hook.py | 3 +- 4 files changed, 259 insertions(+), 71 deletions(-) diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index 5b8b12b9..1b0200fa 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -680,7 +680,8 @@ def remove_vertex(self, vertex: BaseVertex) -> None: raise ValueError("vertex does not belong to this graph") assert vertex.name is not None - for edge in [e for e in self._edgelist if e.v1 is vertex or e.v2 is vertex]: + # its own lists name every edge at it, so no need to search the graph + for edge in vertex._incident_edges(): self.remove_edge(edge) self._vertexlist.remove(vertex) @@ -1702,10 +1703,7 @@ def iscyclic(self) -> bool: return self.ne > self.n - self.nc # DGraph: Kahn's algorithm - indegree = {vertex: 0 for vertex in self} - for e in self.edges(): - assert e.v2 is not None - indegree[e.v2] += 1 + indegree = {vertex: vertex.in_degree for vertex in self} frontier = [vertex for vertex in self if indegree[vertex] == 0] removed = 0 @@ -1726,13 +1724,19 @@ def average_degree(self) -> float: :return: average degree :rtype: float - Average degree is :math:`2 E / N` for an undirected graph and - :math:`E / N` for a directed graph where :math:`E` is the total number of - edges and :math:`N` is the number of vertices. + Average degree is :math:`2 E / N` where :math:`E` is the total number + of edges and :math:`N` is the number of vertices, for an undirected or a + directed graph. It is the mean of :attr:`BaseVertex.degree`, and every + edge has two ends, so the degrees sum to :math:`2 E`. Parallel edges and self-loops are each counted as one edge in - :math:`E`, which is consistent with :meth:`degree`: a self-loop - contributes 2 to the degree sum of an undirected graph. + :math:`E`; a self-loop contributes 2 to the degree sum. + + .. versionchanged:: 1.0 + For a ``DGraph`` this was :math:`E / N`, the mean *out*-degree + (which equals the mean in-degree). It is now :math:`2 E / N`, the + mean of the degree, which is twice the old value. Use + ``g.ne / g.n`` for the previous number. .. runblock:: pycon @@ -1747,12 +1751,7 @@ def average_degree(self) -> float: :seealso: :meth:`degree` """ - if isinstance(self, DGraph): - return len(self.edges()) / self.n - elif isinstance(self, UGraph): - return 2 * len(self.edges()) / self.n - else: - raise TypeError(f"unsupported graph type {type(self).__name__}") + return 2 * self.ne / self.n # --------------------------------------------------------------------------- # @@ -1785,7 +1784,7 @@ def Laplacian(self) -> NDArray: .. note:: - Laplacian always has at least one zero eigenvalue (each row of - ``degree() - adjacency()`` sums to zero, so the all-ones + ``D_out - adjacency()`` sums to zero, so the all-ones vector is always a right null vector). - For an **undirected** graph specifically: the Laplacian is symmetric and positive-semidefinite, and the number of @@ -1801,14 +1800,19 @@ def Laplacian(self) -> NDArray: .. rubric:: For graphs with parallel edges or loops - Both :meth:`degree` and :meth:`adjacency` count edges, so the - identity ``L = degree() - adjacency()`` and the zero row sums hold - unchanged. A self-loop adds the same amount to both and cancels out + Both the out-degree and :meth:`adjacency` count edges, so the + identity ``L = D_out - adjacency()`` and the zero row sums hold + unchanged. A self-loop adds the same amount to both and cancels out of the Laplacian entirely. + .. note:: ``D_out`` is the diagonal matrix of + :attr:`BaseVertex.out_degree`. For an undirected graph that is + :meth:`degree`, but for a directed graph :meth:`degree` is in plus + out, so the Laplacian does *not* use it. + :seealso: :meth:`adjacency` :meth:`incidence` :meth:`degree` """ - return self.degree() - self.adjacency() + return np.diag([v.out_degree for v in self]) - self.adjacency() def connectivity(self, vertices: Iterable[BaseVertex] | None = None) -> list[int]: """ @@ -1820,6 +1824,15 @@ def connectivity(self, vertices: Iterable[BaseVertex] | None = None) -> list[int :return: a list with the number of edges per vertex :rtype: list + This is the :attr:`BaseVertex.degree` of each vertex: the number of + edge *ends* at it, which is the number of edges connected to it except + that a self-loop has two ends and so counts twice. For a directed + graph it counts the edges entering the vertex as well as those leaving. + + .. versionchanged:: 1.0 + For a ``DGraph`` this counted only the edges *leaving* each vertex. + Use ``[v.out_degree for v in g]`` for the previous values. + The average vertex connectivity is:: mean(g.connectivity()) @@ -1843,9 +1856,8 @@ def connectivity(self, vertices: Iterable[BaseVertex] | None = None) -> list[int >>> print(c) - .. note:: This is the diagonal of :meth:`degree`, so parallel edges are - counted individually and a self-loop counts twice for a - ``UGraph`` and once for a ``DGraph``. See the discussion there. + .. note:: This is the diagonal of :meth:`degree`. Parallel edges are + counted individually. :seealso: :meth:`degree` """ @@ -1854,7 +1866,7 @@ def connectivity(self, vertices: Iterable[BaseVertex] | None = None) -> list[int if vertices is None: vertices = self for n in vertices: - c.append(len(n._edgelist)) + c.append(n.degree) return c def degree(self) -> NDArray: @@ -1864,11 +1876,16 @@ def degree(self) -> NDArray: :return: degree matrix :rtype: ndarray(N,N) - This is a diagonal matrix where element ``[i,i]`` is the number - of edges connected to vertex id ``i``. + This is a diagonal matrix where element ``[i,i]`` is the + :attr:`BaseVertex.degree` of vertex ``i``: the number of edge ends at + it, so a self-loop counts twice. For a directed graph that is the + edges entering plus the edges leaving. - .. note:: For a ``DGraph`` only outgoing edges are counted, matching - :attr:`BaseVertex.degree`. + .. versionchanged:: 1.0 + For a ``DGraph`` this was the out-degree matrix, counting only the + edges leaving each vertex. It is now the total, in plus out. The + old matrix is ``np.diag([v.out_degree for v in g])``, which is what + :meth:`Laplacian` uses. .. runblock:: pycon @@ -1887,11 +1904,11 @@ def degree(self) -> NDArray: .. rubric:: For graphs with parallel edges or loops Every edge is counted, so ``k`` parallel edges between two vertices - add ``k`` to the degree of each end. A self-loop adds 2 to the degree - of a ``UGraph`` vertex, since it has two ends, and 1 to the - (out-)degree of a ``DGraph`` vertex. + add ``k`` to the degree of each end. A self-loop adds 2, for both + kinds of graph, since it has two ends. - :seealso: :meth:`adjacency` :meth:`incidence` :meth:`laplacian` + :seealso: :meth:`adjacency` :meth:`incidence` :meth:`Laplacian` + :meth:`connectivity` :meth:`average_degree` :attr:`BaseVertex.degree` """ return np.diag(self.connectivity()) @@ -3020,6 +3037,9 @@ def __init__(self, coord: ArrayLike | None = None, name: str | None = None): 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: BaseGraph | None = None # reference to owning graph # print('BaseVertex init', type(self)) @@ -3119,7 +3139,8 @@ def adjacent(self) -> list[BaseVertex]: >>> g.add_edge(v1, v3) >>> print(v1.adjacent()) - :seealso: :meth:`isadjacent` :meth:`incidences` :meth:`edges` + :seealso: :meth:`isadjacent` :meth:`successors` :meth:`predecessors` + :meth:`incidences` :meth:`out_edges` :meth:`edges` :meth:`BaseGraph.adjacency` """ return [e.next(self) for e in self._edgelist] @@ -3165,12 +3186,33 @@ def isadjacent(self, vertex: BaseVertex) -> bool: def incidences(self) -> list[tuple[BaseVertex, Edge]]: """ - Neighbours and edges of a vertex + Adjacent vertices paired with the edges that lead to them + + :return: a ``(vertex, edge)`` pair for each edge leaving this vertex, + in the order the edges were connected + :rtype: list of (BaseVertex subclass, Edge) + + ``v.incidences()`` is :meth:`adjacent` and :meth:`out_edges` taken + together: the first element of each pair is the vertex at the far end + of the edge, the second is the edge itself, so its :attr:`Edge.cost` + and :attr:`Edge.data` are at hand. It is equivalent to:: - ``v.incidences()`` is a generator that returns a list of incidences, - tuples of (vertex, edge) for all vertices adjacent to the vertex ``v``. + list(zip(v.adjacent(), v.out_edges())) - .. note:: For a directed graph the edges are those leaving this vertex + but is built in a single pass, which is what the path-planning methods + want when they expand a vertex. + + .. note:: + + - In graph-theory terms :meth:`adjacent` is a vertex's *adjacency + list*, :meth:`out_edges` its *incidence list*, and this is the + two joined. + - For a directed graph the pairs are for the edges *leaving* this + vertex. The ones entering it are :meth:`in_edges`, and the + vertices at their tails are :meth:`predecessors`. + - There is one pair per edge, so parallel edges to the same vertex + give repeated vertices with different edges, and an undirected + self-loop gives the pair twice, once for each end. .. runblock:: pycon @@ -3178,10 +3220,16 @@ def incidences(self) -> list[tuple[BaseVertex, Edge]]: >>> g = UGraph() >>> v1 = g.add_vertex(coord=[0,0], name='v1') >>> v2 = g.add_vertex(coord=[1,1], name='v2') - >>> g.add_edge(v1, v2) - >>> print(v1.incidences()) - - :seealso: :meth:`adjacent` :meth:`edges` + >>> v3 = g.add_vertex(coord=[2,2], name='v3') + >>> g.add_edge(v1, v2, cost=1.5) + >>> g.add_edge(v1, v3, cost=2.5) + >>> for vertex, edge in v1.incidences(): + ... print(vertex.name, edge.cost) + + :seealso: :meth:`adjacent` :meth:`isadjacent` :meth:`successors` + :meth:`out_edges` :meth:`in_edges` :meth:`predecessors` + :meth:`edges` :meth:`edgeto` :attr:`degree` + :meth:`BaseGraph.adjacency` """ return [(e.next(self), e) for e in self._edgelist] @@ -3396,6 +3444,9 @@ def edges(self) -> list[Edge]: >>> v2 = g.add_vertex(coord=[1,1], name='v2') >>> g.add_edge(v1, v2) >>> print(v1.edges()) + + :seealso: :meth:`out_edges` :meth:`in_edges` :meth:`adjacent` + :meth:`incidences` :attr:`degree` :meth:`BaseGraph.edges` """ return self._edgelist @@ -3466,30 +3517,149 @@ def degree(self) -> int: :return: degree of the vertex :rtype: int - Returns the number of edges connected to the vertex. + The number of edge ends at this vertex: the edges leaving it plus the + edges entering it, so a self-loop counts twice. The degrees of all the + vertices of a graph sum to twice its number of edges. - .. note:: For a ``DGraph`` only outgoing edges are considered. + For an undirected graph this is the number of incident edges. For a + directed graph it is :attr:`in_degree` + :attr:`out_degree`. + + .. versionchanged:: 1.0 + For a ``DGraph`` this used to count only the edges *leaving* the + vertex. It is now the graph-theory degree, in plus out. Use + :attr:`out_degree` for the previous value. .. runblock:: pycon - >>> from pgraph import UGraph - >>> g = UGraph() + >>> from pgraph import DGraph + >>> g = DGraph() >>> v1 = g.add_vertex(coord=[0,0], name='v1') >>> v2 = g.add_vertex(coord=[1,1], name='v2') >>> v3 = g.add_vertex(coord=[2,2], name='v3') >>> g.add_edge(v1, v2) >>> g.add_edge(v1, v3) - >>> print(v1.degree) + >>> g.add_edge(v3, v1) + >>> print(v1.degree, v1.in_degree, v1.out_degree) .. rubric:: For graphs with parallel edges or loops - Every edge is counted, so ``k`` parallel edges to the same neighbour - add ``k``. A self-loop adds 2 for a ``UGraph`` vertex, since it has two - ends, and 1 for a ``DGraph`` vertex. + Every edge is counted, so ``k`` parallel edges to the same vertex add + ``k``. A self-loop adds 2, for both kinds of graph: it has two ends. + + :seealso: :attr:`in_degree` :attr:`out_degree` :meth:`edges` + :meth:`BaseGraph.degree` :meth:`BaseGraph.connectivity` + """ + return len(self._edgelist) + len(self._inlist) + + @property + def out_degree(self) -> int: + """ + Number of edges leaving the vertex + + :return: out-degree of the vertex + :rtype: int + + For an undirected graph every incident edge both leaves and enters, so + this equals :attr:`degree`. - :seealso: :meth:`edges` + :seealso: :attr:`in_degree` :attr:`degree` :meth:`out_edges` """ - return len(self.edges()) + return len(self._edgelist) + + @property + def in_degree(self) -> int: + """ + Number of edges entering the vertex + + :return: in-degree of the vertex + :rtype: int + + For an undirected graph every incident edge both leaves and enters, so + this equals :attr:`degree`. + + :seealso: :attr:`out_degree` :attr:`degree` :meth:`in_edges` + """ + return len(self._incoming()) + + def _incoming(self) -> list[Edge]: + """The edges that end here (private method)""" + return self._inlist + + def out_edges(self) -> list[Edge]: + """ + The edges leaving the vertex + + :return: the outgoing edges, in the order they were connected + :rtype: list of ``Edge`` + + This is a new list, and is the same as :meth:`edges`: for an undirected + graph every incident edge is outgoing. + + :seealso: :meth:`in_edges` :meth:`edges` :meth:`adjacent` :meth:`successors` + :meth:`incidences` :attr:`out_degree` + """ + return list(self._edgelist) + + def in_edges(self) -> list[Edge]: + """ + The edges entering the vertex + + :return: the incoming edges, in the order they were connected + :rtype: list of ``Edge`` + + This is a new list. For an undirected graph every incident edge is + incoming, so it equals :meth:`edges`. + + :seealso: :meth:`out_edges` :meth:`edges` :meth:`predecessors` :attr:`in_degree` + """ + return list(self._incoming()) + + def successors(self) -> list[BaseVertex]: + """ + Vertices that this vertex has an edge to + + :return: the vertices at the head of the outgoing edges + :rtype: list of BaseVertex subclass + + This is :meth:`adjacent`, under its directed-graph name. + + :seealso: :meth:`predecessors` :meth:`adjacent` :meth:`incidences` + :meth:`out_edges` + """ + return self.adjacent() + + def predecessors(self) -> list[BaseVertex]: + """ + Vertices that have an edge to this vertex + + :return: the vertices at the tail of the incoming edges + :rtype: list of BaseVertex subclass + + There is one entry per edge, so a vertex joined by ``k`` parallel edges + appears ``k`` times. For an undirected graph this is :meth:`adjacent`. + + .. runblock:: pycon + + >>> from pgraph import DGraph + >>> g = DGraph() + >>> v1 = g.add_vertex(coord=[0,0], name='v1') + >>> v2 = g.add_vertex(coord=[1,1], name='v2') + >>> v3 = g.add_vertex(coord=[2,2], name='v3') + >>> g.add_edge(v1, v3) + >>> g.add_edge(v2, v3) + >>> print(v3.predecessors()) + + :seealso: :meth:`successors` :meth:`adjacent` :meth:`in_edges` + :attr:`in_degree` + """ + return [e.next(self) for e in self._incoming()] + + def _incident_edges(self) -> list[Edge]: + """Every distinct edge at this vertex, either end (private method)""" + seen: dict[int, Edge] = {} + for e in (*self._edgelist, *self._incoming()): + seen.setdefault(id(e), e) + return list(seen.values()) @property def x(self) -> float: @@ -3645,6 +3815,10 @@ def _attach(self, edge: Edge, dest: BaseVertex) -> None: self._edgelist.append(edge) dest._edgelist.append(edge) + def _incoming(self) -> list[Edge]: + """The edges that end here: for an undirected vertex, all of them""" + return self._edgelist + def _detach(self, edge: Edge, dest: BaseVertex) -> None: """ Remove an undirected edge from both ends (private method) @@ -3694,21 +3868,23 @@ class DVertex(BaseVertex): def _attach(self, edge: Edge, dest: BaseVertex) -> None: """ - Record a directed edge at its source only (private method) + Record a directed edge at both ends (private method) - The edge is added only to *this* vertex's edge list, so it is - discoverable only from the start of the directed edge. + The edge goes in this vertex's list of outgoing edges and in + ``dest``'s list of incoming ones. """ self._edgelist.append(edge) + dest._inlist.append(edge) def _detach(self, edge: Edge, dest: BaseVertex) -> None: """ - Remove a directed edge from its source (private method) - - ``dest`` never held it. + Remove a directed edge from its source and from where it ends + (private method) """ if edge in self._edgelist: self._edgelist.remove(edge) + if edge in dest._inlist: + dest._inlist.remove(edge) # UGraph/DGraph declare their _vertex_cls here, rather than in their own diff --git a/tests/test_graph.py b/tests/test_graph.py index f9f7f15f..94fc4f34 100644 --- a/tests/test_graph.py +++ b/tests/test_graph.py @@ -393,7 +393,8 @@ def test_properties(self): self.assertEqual(g.n, 4) self.assertEqual(g.ne, 2) - self.assertEqual(g.average_degree(), 0.5) + # degrees sum to twice the edges for either kind of graph: 4 / 4 + self.assertEqual(g.average_degree(), 1.0) def test_contains(self): @@ -894,17 +895,20 @@ def test_matrices(self): nt.assert_array_equal(A, expected_A) self.assertFalse(np.array_equal(A, A.T)) # not symmetric, directed - # out-degree only: v0 has 2 outgoing, v1 has 1, v2 has 0 (even - # though v2 has two *incoming* edges) + # degree is in + out: v0 has 2 out and 0 in, v1 has 1 and 1, v2 has + # 0 and 2 -- two edge ends each D = g.degree() - nt.assert_array_equal(D, np.diag([2, 1, 0])) + nt.assert_array_equal(D, np.diag([2, 2, 2])) + # the directed Laplacian uses the OUT-degree, D_out - A, whose rows + # sum to zero; it is not degree() - A for a directed graph L = g.Laplacian() expected_L = np.diag([2, 1, 0]) - expected_A nt.assert_array_equal(L, expected_L) + nt.assert_array_equal(L.sum(axis=1), np.zeros(3)) - # incidence marks both endpoints regardless of direction, so unlike - # degree() every vertex here touches exactly 2 edges + # incidence marks both endpoints regardless of direction, so it + # agrees with the total degree: every vertex touches exactly 2 edges I = g.incidence() self.assertEqual(I.shape, (g.n, g.ne)) nt.assert_array_equal(I.sum(axis=0), np.full(g.ne, 2)) diff --git a/tests/test_multigraph.py b/tests/test_multigraph.py index a309b6de..c1883d2a 100644 --- a/tests/test_multigraph.py +++ b/tests/test_multigraph.py @@ -108,15 +108,21 @@ def check_matrices_consistent(self): """Relationships that must hold for *any* edge multiset.""" g = self.g deg = np.array(g.connectivity()) + out = np.array([v.out_degree for v in g]) A = g.adjacency() - np.testing.assert_array_equal(A.sum(axis=1), deg) + # a row of the adjacency matrix counts the edges that leave the vertex + np.testing.assert_array_equal(A.sum(axis=1), out) if not self.directed: np.testing.assert_array_equal(A, A.T) + np.testing.assert_array_equal(out, deg) + # the Laplacian is D_out - A, so each row sums to zero for any graph L = g.Laplacian() - np.testing.assert_array_equal(L, g.degree() - A) + np.testing.assert_array_equal(L, np.diag(out) - A) np.testing.assert_allclose(L.sum(axis=1), 0) + # and degrees sum to twice the edges (every edge has two ends) + self.assertEqual(deg.sum(), 2 * g.ne) I = g.incidence() self.assertEqual(I.shape, (g.n, g.ne)) @@ -284,10 +290,11 @@ def test_parallel_edges_bfs(self): class TestDGraphMultigraph(_Multigraph, unittest.TestCase): Graph = DGraph directed = True - deg_base = [1, 1, 1, 0] # out-degree only - deg_loop = [1, 2, 1, 0] # a loop contributes 1 - deg_parallel = [3, 1, 1, 0] - deg_both = [3, 2, 1, 0] + # degree is in + out, so a path's degrees are those of the undirected path + deg_base = [1, 2, 2, 1] + deg_loop = [1, 4, 2, 1] # a loop has two ends: one out, one in + deg_parallel = [3, 4, 2, 1] + deg_both = [3, 6, 2, 1] loop_colsum = 1 loop_adjacency = 1 diff --git a/tests/test_vertex_hook.py b/tests/test_vertex_hook.py index fea4d8e9..771bb34a 100644 --- a/tests/test_vertex_hook.py +++ b/tests/test_vertex_hook.py @@ -126,7 +126,8 @@ def test_edge_is_held_by_the_source_only(self): def test_self_loop_is_held_once(self): e = self.a.connect(self.a) self.assertEqual(self.a.edges(), [e]) - self.assertEqual(self.a.degree, 1) + self.assertEqual(self.a.out_degree, 1) # held once as an outgoing edge + self.assertEqual(self.a.degree, 2) # and once as an incoming one class TestHookIsTheExtensionPoint(unittest.TestCase): From 8ac3fc7b70e7d97bb4fb0dd46034feef83095b23 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 08:26:54 +1000 Subject: [PATCH 4/7] feat: graph.validate() reports inconsistencies between a graph's records A graph keeps the same facts in several places: its set of edges, each vertex's outgoing (and, if directed, incoming) edge lists, each edge's two vertices, each vertex's pointer back to its graph, and the name table. validate() returns a list of the ways they disagree, empty when consistent. It does not change the graph, costs O(V+E), reports every problem rather than the first, and does not raise on corruption, including stray objects in the containers. For tests, debugging, and code that reaches into the internals; a graph built and edited through the public methods is always valid. 27 deliberate corruptions, one per kind of record, are each detected and named; random edit sequences stay valid. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 184 ++++++++++++++++++++++++++++++++ tests/test_validate.py | 235 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 419 insertions(+) create mode 100644 tests/test_validate.py diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index 1b0200fa..ee395ea1 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -306,6 +306,190 @@ def edge_class(self) -> type[Edge]: """ return self._edge_class + def validate(self) -> list[str]: + """ + Check that the graph's records agree with each other + + :return: a description of each inconsistency found, empty if the graph + is consistent + :rtype: list of str + + A graph keeps the same facts in several places: its set of edges, each + vertex's list of outgoing edges (and, for a directed graph, of incoming + ones), each edge's two vertices, each vertex's pointer back to its + graph, and the table of vertex names. The library keeps them in step, + so a graph built and edited through the public methods is always + valid. This is a check for tests, for debugging, and for code that + reaches into the internals. + + It does not change the graph, and takes time proportional to the + number of vertices plus edges. It reports every inconsistency, not + only the first. + + The checks are: + + - the vertex list holds only vertices, once each, and the edge set + holds only edges + - every edge in the graph has two vertices, both in this graph, and is + an instance of :attr:`edge_class` + - every vertex points back at this graph, is an instance of + :attr:`vertex_class`, is in the name table under its own name, and + has a coordinate of the required length if ``dim`` was given + - every entry of the name table is a vertex of this graph and is + filed under its own name + - each edge is in the right vertex lists, the right number of times: + both ends for an undirected edge (twice at one vertex for a + self-loop), the source's outgoing and the target's incoming list for + a directed edge + - no vertex list holds an edge that is not in the graph + + .. runblock:: pycon + + >>> from pgraph import UGraph + >>> g = UGraph() + >>> v1 = g.add_vertex(name='v1') + >>> v2 = g.add_vertex(name='v2') + >>> g.add_edge(v1, v2) + >>> print(g.validate()) + + :seealso: :meth:`remove_edge` :meth:`remove_vertex` + """ + from collections import Counter + + problems: list[str] = [] + directed = isinstance(self, DGraph) + + # ---- the containers themselves must hold the right kind of thing; a + # stray object is reported and set aside so the checks below can run + vertices: list[BaseVertex] = [] + for entry in self._vertexlist: + if isinstance(entry, BaseVertex): + vertices.append(entry) + else: + problems.append(f"the vertex list holds {entry!r}, which is not a vertex") + graph_edges: list[Edge] = [] + for member in self._edgelist: + if isinstance(member, Edge): + graph_edges.append(member) + else: + problems.append(f"the edge set holds {member!r}, which is not an edge") + + # ---- vertices + members = {id(v) for v in vertices} + for vid, n in Counter(id(v) for v in vertices).items(): + if n > 1: + v = next(x for x in vertices if id(x) == vid) + problems.append(f"vertex {v.name!r} is listed {n} times in the vertex list") + for key, v in self._vertexdict.items(): + if not isinstance(v, BaseVertex) or id(v) not in members: + problems.append( + f"name table entry {key!r} refers to a vertex that is not in the graph" + ) + elif v.name != key: + problems.append( + f"name table entry {key!r} refers to vertex {v.name!r}, " + "which has a different name" + ) + for v in vertices: + if v._graph is not self: + problems.append(f"vertex {v.name!r} does not point back at this graph") + if v.name is None or self._vertexdict.get(v.name) is not v: + problems.append( + f"vertex {v.name!r} is not in the name table under its own name" + ) + if not isinstance(v, self._vertex_class): + problems.append( + f"vertex {v.name!r} is a {type(v).__name__}, not a " + f"{self._vertex_class.__name__}" + ) + if ( + self._dim is not None + and v.coord is not None + and len(v.coord) != self._dim + ): + problems.append( + f"vertex {v.name!r} has a coordinate of length " + f"{len(v.coord)} but the graph requires dim={self._dim}" + ) + if not directed and v._inlist: + problems.append( + f"vertex {v.name!r} has {len(v._inlist)} incoming edges " + "recorded, but an undirected vertex does not use that list" + ) + + # ---- edges, and what each vertex list should then contain + expected_out: Counter[tuple[int, int]] = Counter() + expected_in: Counter[tuple[int, int]] = Counter() + edges: dict[int, Edge] = {} + in_graph = {id(e) for e in graph_edges} + for e in graph_edges: + edges[id(e)] = e + if e.v1 is None or e.v2 is None: + problems.append(f"edge {e!r} is in the graph but has no vertices") + continue + for end in (e.v1, e.v2): + if id(end) not in members: + problems.append( + f"edge {e!r} joins vertex {end.name!r}, which is not in this graph" + ) + if not isinstance(e, self._edge_class): + problems.append( + f"edge {e!r} is a {type(e).__name__}, not a {self._edge_class.__name__}" + ) + expected_out[(id(e.v1), id(e))] += 1 + if directed: + expected_in[(id(e.v2), id(e))] += 1 + else: + expected_out[(id(e.v2), id(e))] += 1 + + actual_out: Counter[tuple[int, int]] = Counter() + actual_in: Counter[tuple[int, int]] = Counter() + owner = {id(v): v for v in vertices} + for v in vertices: + for listname, held, counter in ( + ("outgoing" if directed else "edge", v._edgelist, actual_out), + ("incoming", v._inlist, actual_in), + ): + for e in held: + if not isinstance(e, Edge): + problems.append( + f"vertex {v.name!r} holds {e!r} in its {listname} list, " + "which is not an edge" + ) + continue + counter[(id(v), id(e))] += 1 + edges.setdefault(id(e), e) + + def report(expected: Counter, actual: Counter, listname: str) -> None: + for key in expected.keys() | actual.keys(): + vid, eid = key + want, have = expected[key], actual[key] + if want == have: + continue + v, e = owner.get(vid), edges[eid] + vname = v.name if v is not None else "?" + if eid not in in_graph: + problems.append( + f"vertex {vname!r} holds edge {e!r} in its {listname} " + "list, but that edge is not in the graph" + ) + elif have < want: + problems.append( + f"edge {e!r} is missing from the {listname} list of " + f"vertex {vname!r} (found {have}, expected {want})" + ) + else: + problems.append( + f"edge {e!r} appears {have} times in the {listname} list " + f"of vertex {vname!r}, expected {want}" + ) + + report(expected_out, actual_out, "outgoing" if directed else "edge") + if directed: + report(expected_in, actual_in, "incoming") + + return problems + def copy(self) -> BaseGraph: """ Deepcopy of graph diff --git a/tests/test_validate.py b/tests/test_validate.py new file mode 100644 index 00000000..caa8dae5 --- /dev/null +++ b/tests/test_validate.py @@ -0,0 +1,235 @@ +""" +``graph.validate()`` checks that a graph's records agree with each other and +returns a list of problems, empty if it is consistent. + +A graph keeps the same facts in several places: the set of edges, each +vertex's lists of outgoing (and, if directed, incoming) edges, each edge's two +vertices, each vertex's pointer back to its graph, and the name table. The +library keeps them in step, so a valid graph is the normal case; ``validate`` +is for tests, debugging, and code that reaches into the internals. +""" + +import random +import unittest + +from pgraph import DGraph, Edge, UGraph, UVertex + + +def poison(edge, name, value): + """Break an edge's endpoint by hand, whether v1/v2 are plain attributes or + read-only properties over _v1/_v2 -- the point is to corrupt the record.""" + try: + setattr(edge, name, value) + except AttributeError: + setattr(edge, "_" + name, value) + + +def build(Graph): + g = Graph() + a, b, c, d = (g.add_vertex([i, 0], name=n) for i, n in enumerate("abcd")) + a.connect(b) + a.connect(b) # parallel + b.connect(c) + c.connect(c) # self-loop + if Graph is DGraph: + c.connect(a) + return g, a, b, c, d + + +class TestValidGraphs(unittest.TestCase): + def test_returns_a_list_of_strings(self): + for Graph in (UGraph, DGraph): + r = build(Graph)[0].validate() + self.assertIsInstance(r, list) + self.assertEqual(r, []) + + def test_empty_graph(self): + self.assertEqual(UGraph().validate(), []) + self.assertEqual(DGraph().validate(), []) + + def test_loops_and_parallel_edges_are_valid(self): + for Graph in (UGraph, DGraph): + g, *_ = build(Graph) + self.assertEqual(g.validate(), [], Graph.__name__) + + def test_stays_valid_through_removals(self): + for Graph in (UGraph, DGraph): + g, a, b, c, d = build(Graph) + g.remove_edge(a.edges()[0]) + self.assertEqual(g.validate(), []) + g.remove_vertex(c) + self.assertEqual(g.validate(), []) + g.remove_vertex(a) + self.assertEqual(g.validate(), []) + + def test_random_edit_sequences(self): + rng = random.Random(3) + for Graph in (UGraph, DGraph): + g = Graph() + vs = [g.add_vertex([i, 0]) for i in range(7)] + for _ in range(80): + if rng.random() < 0.6 or g.ne == 0: + rng.choice(vs).connect(rng.choice(vs)) + else: + g.remove_edge(rng.choice(sorted(g.edges(), key=id))) + self.assertEqual(g.validate(), []) + + def test_user_vertex_and_edge_classes(self): + class V(UVertex): + pass + + class E(Edge): + pass + + g = UGraph(vertex_class=V, edge_class=E) + a, b = g.add_vertex([0, 0]), g.add_vertex([1, 0]) + g.add_edge(a, b) + self.assertEqual(g.validate(), []) + + def test_does_not_change_the_graph(self): + g, *_ = build(DGraph) + before = (g.n, g.ne, [list(v._edgelist) for v in g], [list(v._inlist) for v in g]) + g.validate() + g.validate() + after = (g.n, g.ne, [list(v._edgelist) for v in g], [list(v._inlist) for v in g]) + self.assertEqual(before, after) + + +class TestFindsCorruption(unittest.TestCase): + """each case breaks one record by hand and expects it to be named""" + + def problems(self, g): + r = g.validate() + self.assertTrue(r, "expected a problem to be reported") + self.assertTrue(all(isinstance(p, str) for p in r)) + return " | ".join(r) + + def test_edge_missing_from_a_vertex_list(self): + for Graph in (UGraph, DGraph): + g, a, b, c, d = build(Graph) + a._edgelist.pop() + self.assertIn("missing", self.problems(g), Graph.__name__) + + def test_edge_held_by_a_vertex_but_not_in_the_graph(self): + for Graph in (UGraph, DGraph): + g, a, b, c, d = build(Graph) + ghost = Edge(a, d) + a._edgelist.append(ghost) + self.assertIn("not in the graph", self.problems(g), Graph.__name__) + + def test_edge_in_graph_without_vertices(self): + g, a, b, c, d = build(UGraph) + e = a.edges()[0] + poison(e, "v1", None) + self.assertIn("no vertices", self.problems(g)) + + def test_edge_whose_vertices_disagree_with_the_lists(self): + g, a, b, c, d = build(UGraph) + e = a.edges()[0] + poison(e, "v1", d) # now claims d, but a's list still holds it + msg = self.problems(g) + self.assertIn("'d'", msg) + + def test_directed_incoming_list_out_of_step(self): + g, a, b, c, d = build(DGraph) + b._inlist.pop() + self.assertIn("missing", self.problems(g)) + + def test_undirected_vertex_must_not_use_the_incoming_list(self): + g, a, b, c, d = build(UGraph) + b._inlist.append(a.edges()[0]) + self.assertIn("incoming", self.problems(g)) + + def test_vertex_does_not_point_back(self): + g, a, b, c, d = build(UGraph) + a._graph = None + self.assertIn("'a'", self.problems(g)) + + def test_vertex_missing_from_the_name_table(self): + g, a, b, c, d = build(UGraph) + del g._vertexdict["d"] + self.assertIn("'d'", self.problems(g)) + + def test_vertex_of_the_wrong_class(self): + class V(UVertex): + pass + + g = UGraph(vertex_class=V) + g.add_vertex([0, 0], name="a") + g._vertexlist.append(UVertex([1, 0], name="plain")) + g._vertexlist[-1]._graph = g + g._vertexdict["plain"] = g._vertexlist[-1] + self.assertIn("plain", self.problems(g)) + + def test_wrong_coordinate_length(self): + g = UGraph(dim=3) + v = g.add_vertex([0, 0, 0], name="a") + v.coord = v.coord[:2] + self.assertIn("dim", self.problems(g)) + + def test_edge_to_a_vertex_in_another_graph(self): + g, a, b, c, d = build(UGraph) + h = UGraph() + x = h.add_vertex(name="x") + poison(a.edges()[0], "v2", x) + self.assertIn("'x'", self.problems(g)) + + 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) + a._edgelist.append("junk") + msg = self.problems(g) + for word in ("junk", "42", "not a vertex", "not an edge"): + self.assertIn(word, msg) + + def test_junk_in_a_directed_incoming_list(self): + g, a, b, c, d = build(DGraph) + b._inlist.append(3) + self.assertIn("incoming", self.problems(g)) + + def test_vertex_listed_twice(self): + g, a, b, c, d = build(UGraph) + g._vertexlist.append(a) + self.assertIn("'a' is listed 2 times", self.problems(g)) + + def test_name_table_entries_are_named(self): + g, a, b, c, d = build(UGraph) + g._vertexdict["zz"] = a # a filed under a second, wrong name + self.assertIn("'zz'", self.problems(g)) + del g._vertexdict["zz"] + g._vertexdict["x"] = UGraph().add_vertex(name="x") # not in this graph + self.assertIn("'x'", self.problems(g)) + + def test_edge_of_the_wrong_class(self): + class MyEdge(Edge): + pass + + 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) + 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]) + self.assertIn("not in the graph", self.problems(g)) + + def test_self_loop_held_once_instead_of_twice(self): + g, a, b, c, d = build(UGraph) + c._edgelist.pop() + self.assertIn("missing", self.problems(g)) + + def test_reports_every_problem_not_just_the_first(self): + g, a, b, c, d = build(UGraph) + a._edgelist.pop() + del g._vertexdict["d"] + self.assertGreaterEqual(len(g.validate()), 2) + + +if __name__ == "__main__": + unittest.main() From 4613e04e7152c8958b794d97fe04e39f7c66a3af Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 08:26:55 +1000 Subject: [PATCH 5/7] refactor: public names say what is API; label and name cannot corrupt a graph label read-only. It is the component number, computed by the graph. name assigning to the name of a vertex that is in a graph updates the graph's name table, refuses a name already in use (ValueError) and refuses to clear it. It left the table stale before, so g[new] failed. exporters use the public API: out_edges() instead of a vertex's private _edgelist, and get_exporter(name) instead of the private table. docs BaseVertex no longer lists _edgelist as an attribute. Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 107 ++++++++++++++++---- src/pgraph/exporters.py | 23 ++++- tests/test_attribute_hygiene.py | 169 ++++++++++++++++++++++++++++++++ 3 files changed, 275 insertions(+), 24 deletions(-) create mode 100644 tests/test_attribute_hygiene.py diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index ee395ea1..77abd3a4 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -1736,14 +1736,7 @@ def export( filename.write(image.decode("utf-8")) return None - try: - func = exporters._TEXT[format] - except KeyError: - raise ValueError( - f"unknown export format {format!r}, expected one of " - f"{exporters.formats()}" - ) from None - text = func(self, **options) + text = exporters.get_exporter(format)(self, **options) if filename is None: return text @@ -2712,7 +2705,7 @@ def _graphcolor(self) -> None: # clear all the labels for vertex in self: - vertex.label = None + vertex._label = None vertex._connectivitychange = False lastlabel: int | None = None @@ -2725,7 +2718,7 @@ def _graphcolor(self) -> None: q = [v] # initialize frontier while len(q) > 0: v = q.pop() # expand v - v.label = label + v._label = label for n in v.adjacent(): if n.label is None: q.append(n) @@ -2771,7 +2764,7 @@ def _graphcolor(self) -> int | None: # clear all the labels for vertex in self: - vertex.label = None + vertex._label = None vertex._connectivitychange = False # initial labeling pass @@ -2783,12 +2776,12 @@ def _graphcolor(self) -> int | None: for n in v.adjacent(): if n.label is not None: # neighbour has a label - v.label = n.label + v._label = n.label break if v.label is None: # still not labeled, assign a new label - v.label = nextlabel + v._label = nextlabel nextlabel += 1 label = v.label @@ -2798,7 +2791,7 @@ def _graphcolor(self) -> int | None: for n in v.adjacent(): if n.label is None: # neighbour has no label, give it this one - n.label = label + n._label = label elif label != n.label: # label clash, note it for merging assert n.label is not None @@ -2811,14 +2804,14 @@ def _graphcolor(self) -> int | None: assert vlabel is not None while vlabel in merge: vlabel = merge[vlabel] - v.label = vlabel + v._label = vlabel unique.add(vlabel) final = {u: i for i, u in enumerate(unique)} for v in self: vlabel = v.label assert vlabel is not None - v.label = final[vlabel] + v._label = final[vlabel] return len(unique) else: @@ -3184,11 +3177,15 @@ class BaseVertex: Base class for vertices of directed and non-directed graphs. Each vertex has: - - ``name`` - - ``label`` an int indicating which graph component contains it - - ``_edgelist`` a list of edge objects that connect this vertex to others + - ``name`` which is its key in the graph, so renaming a vertex that is + in a graph updates the graph + - ``label`` an int indicating which graph component contains it, + read-only: the graph assigns it - ``coord`` the coordinate in an embedded graph (optional) + and its edges are available from :meth:`edges`, :meth:`out_edges` and + :meth:`in_edges`. + :seealso: :class:`UVertex` :class:`DVertex` :class:`Edge` """ @@ -3217,8 +3214,8 @@ def __init__(self, coord: ArrayLike | None = None, name: str | None = None): self.coord = None else: self.coord = np.r_[coord] - self.name = name - self.label: int | None = None + 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 @@ -3845,6 +3842,74 @@ def _incident_edges(self) -> list[Edge]: seen.setdefault(id(e), e) return list(seen.values()) + @property + def name(self) -> str | None: + """ + Name of the vertex + + :return: the name + :rtype: str or None + + The name is the vertex's key in its graph: ``g[name]`` and ``name in + g`` look it up. A vertex that is not in a graph can be given any name, + or none. For one that is in a graph: + + - assigning a new name updates the graph's name table, so the vertex + is then found under the new name and not the old + - a name already used by another vertex in the graph raises + ``ValueError`` and changes nothing + - the name cannot be cleared + + .. versionchanged:: 1.0 + Renaming a vertex in a graph used to leave the graph's name table + stale, so lookups by the new name failed. + + :seealso: :meth:`BaseGraph.add_vertex` + """ + return self._name + + @name.setter + def name(self, value: str | None) -> None: + graph = self._graph + old = self._name + if ( + graph is not None + and old is not None + and graph._vertexdict.get(old) is self + ): + if value is None: + raise ValueError("a vertex in a graph must have a name") + if value != old: + if value in graph._vertexdict: + raise ValueError( + f"the graph already has a vertex named {value!r}" + ) + del graph._vertexdict[old] + graph._vertexdict[value] = self + self._name = value + + @property + def label(self) -> int | None: + """ + Number of the graph component that contains the vertex + + :return: component number, counted from 0, or None if the components + have not been found yet + :rtype: int or None + + Assigned by the graph when its components are found, which happens when + :attr:`BaseGraph.nc`, :meth:`BaseGraph.component` or + :meth:`BaseGraph.samecomponent` is used after the graph has changed. + It is read-only. + + .. versionchanged:: 1.0 + Read-only. It was a plain attribute, but it is computed from the + graph's structure and meaningless to assign. + + :seealso: :attr:`BaseGraph.nc` :meth:`BaseGraph.component` + """ + return self._label + @property def x(self) -> float: """ diff --git a/src/pgraph/exporters.py b/src/pgraph/exporters.py index 5cc28744..52ebc435 100644 --- a/src/pgraph/exporters.py +++ b/src/pgraph/exporters.py @@ -75,6 +75,23 @@ def decorator(func: TextExporter) -> TextExporter: return decorator +def get_exporter(name: str) -> TextExporter: + """ + Look up a text exporter by format name + + :param name: format name, eg. ``"dot"`` + :return: the function ``f(graph, **options) -> str`` + :raises ValueError: no text exporter has that name; the message lists those + that do + """ + try: + return _TEXT[name] + except KeyError: + raise ValueError( + f"unknown export format {name!r}, expected one of {formats()}" + ) from None + + def formats() -> list[str]: """ Names of all supported export formats @@ -126,8 +143,8 @@ def _ordered_edges(graph: BaseGraph) -> list[tuple[Edge, BaseVertex, BaseVertex] ``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 edge - list, keeping the first sight of every edge. Identity is by object, not by + 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. @@ -136,7 +153,7 @@ def _ordered_edges(graph: BaseGraph) -> list[tuple[Edge, BaseVertex, BaseVertex] seen: set[int] = set() edges: list[tuple[Edge, BaseVertex, BaseVertex]] = [] for vertex in graph: - for e in vertex._edgelist: + for e in vertex.out_edges(): if id(e) in seen or e.v1 is None or e.v2 is None: continue seen.add(id(e)) diff --git a/tests/test_attribute_hygiene.py b/tests/test_attribute_hygiene.py new file mode 100644 index 00000000..65ea89cf --- /dev/null +++ b/tests/test_attribute_hygiene.py @@ -0,0 +1,169 @@ +""" +Names say what is API and what is not, and the public ones cannot be used to +corrupt a graph. + +- ``label`` is the component number, computed by the graph: read-only +- ``name`` is the key in the graph's name table: renaming a vertex that is in a + graph updates the table, and refuses a name that is already taken +- ``BaseGraph`` is the public name of the common base class (it appears in + every signature and in users' type hints) +- the exporters use the public vertex API, and look formats up through a + function rather than reaching into a private table +""" + +import unittest + +from pgraph import BaseGraph, BaseVertex, DGraph, UGraph, UVertex, exporters + + +class TestLabelIsReadOnly(unittest.TestCase): + def setUp(self): + self.g = UGraph() + self.a, self.b, self.c = (self.g.add_vertex(name=n) for n in "abc") + self.a.connect(self.b) # c is a second component + + def test_cannot_assign(self): + self.g.nc + with self.assertRaises(AttributeError): + self.a.label = 7 + + def test_still_computed(self): + self.assertEqual(self.g.nc, 2) + self.assertEqual(self.a.label, self.b.label) + self.assertNotEqual(self.a.label, self.c.label) + self.assertEqual({v.label for v in self.g}, {0, 1}) + + def test_none_until_the_components_are_found(self): + self.assertIsNone(UVertex(name="loose").label) + + def test_directed_graph_too(self): + g = DGraph() + a, b, c = (g.add_vertex(name=n) for n in "abc") + a.connect(b) + self.assertEqual(g.nc, 2) + with self.assertRaises(AttributeError): + b.label = 0 + self.assertEqual(a.label, b.label) + + +class TestRename(unittest.TestCase): + def setUp(self): + self.g = UGraph() + self.a, self.b = self.g.add_vertex(name="a"), self.g.add_vertex(name="b") + self.a.connect(self.b, cost=1) + + def test_rename_updates_the_name_table(self): + self.a.name = "alpha" + self.assertIs(self.g["alpha"], self.a) + self.assertNotIn("a", self.g) + self.assertIn("alpha", self.g) + self.assertEqual(self.g.validate(), []) + + def test_graph_methods_follow_the_new_name(self): + self.a.name = "alpha" + path, cost, _ = self.g.path_UCS("alpha", "b") + self.assertEqual([v.name for v in path], ["alpha", "b"]) + + def test_name_already_taken(self): + with self.assertRaises(ValueError): + self.a.name = "b" + self.assertEqual(self.a.name, "a") # unchanged + self.assertIs(self.g["a"], self.a) + self.assertEqual(self.g.validate(), []) + + def test_same_name_is_a_no_op(self): + self.a.name = "a" + self.assertIs(self.g["a"], self.a) + self.assertEqual(self.g.validate(), []) + + def test_cannot_clear_the_name_of_a_vertex_in_a_graph(self): + with self.assertRaises(ValueError): + self.a.name = None + self.assertEqual(self.a.name, "a") + + def test_before_adding_is_just_assignment(self): + v = UVertex([0, 0]) + v.name = "free" + v.name = None + v.name = "again" + self.assertEqual(v.name, "again") + g = UGraph() + g.add_vertex(v) + self.assertIs(g["again"], v) + + def test_after_removal_is_just_assignment(self): + self.g.remove_vertex(self.a) + self.a.name = "b" # no longer in the graph, so no clash + self.assertEqual(self.a.name, "b") + self.assertEqual(self.g.validate(), []) + + +class TestValidateAndRename(unittest.TestCase): + """validate() is what notices a name table that is out of step""" + + def test_name_changed_behind_the_graph_s_back(self): + # renaming through .name updates the table; writing the private field + # does not, and validate() says so + g = UGraph() + a = g.add_vertex(name="a") + g.add_vertex(name="b") + a.name = "renamed" + self.assertEqual(g.validate(), []) # the public route stays valid + a._name = "sneaky" + msg = " | ".join(g.validate()) + self.assertTrue(msg) + self.assertIn("sneaky", msg) + self.assertIn("renamed", msg) + + + +class TestPublicBaseNames(unittest.TestCase): + def test_basegraph_is_exported(self): + self.assertTrue(issubclass(UGraph, BaseGraph)) + self.assertTrue(issubclass(DGraph, BaseGraph)) + self.assertIsInstance(UGraph(), BaseGraph) + + def test_basevertex_is_exported(self): + self.assertTrue(issubclass(UVertex, BaseVertex)) + + def test_the_unreleased_private_name_is_gone(self): + import pgraph.PGraph as module + + self.assertFalse(hasattr(module, "_BaseGraph")) + + def test_the_class_reports_its_public_name(self): + self.assertEqual(BaseGraph.__name__, "BaseGraph") + + def test_a_private_attribute_is_not_documented_as_api(self): + self.assertNotIn("_edgelist", BaseVertex.__doc__ or "") + + +class TestExporters(unittest.TestCase): + def test_get_exporter(self): + for name in ("dot", "mermaid", "graphml", "elk"): + self.assertTrue(callable(exporters.get_exporter(name))) + + def test_unknown_exporter(self): + with self.assertRaises(ValueError) as cm: + exporters.get_exporter("nonesuch") + self.assertIn("dot", str(cm.exception)) # lists what is available + + def test_export_uses_it(self): + g = UGraph() + g.add_edge(g.add_vertex(name="a"), g.add_vertex(name="b")) + self.assertEqual(g.export(format="dot"), exporters.get_exporter("dot")(g)) + + def test_edge_order_follows_the_public_api(self): + # the order is vertices in creation order, each vertex's out_edges() + g = DGraph() + a, b, c = (g.add_vertex(name=n) for n in "abc") + e1 = b.connect(c) + e2 = a.connect(b) + e3 = a.connect(c) + listed = [e for e, _, _ in exporters._ordered_edges(g)] + self.assertEqual(listed, [e2, e3, e1]) + self.assertEqual(listed, [e for v in g for e in v.out_edges()]) + + +if __name__ == "__main__": + unittest.main() From 8cf0d2db35329770cd0a1b8176c2de17a97b552b Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 08:26:56 +1000 Subject: [PATCH 6/7] fix: add_vertex refuses a duplicate name, and chooses a free default name Names are how g[name] finds a vertex, so they must be unique in a graph. - adding a vertex whose name is already used raised nothing and replaced the earlier vertex in the name table, leaving it in the graph but unreachable by name; it now raises ValueError and changes nothing - the default name #N used the number of vertices, so after a removal it could equal an existing name (remove #0 from #0 #1 #2, add one: #2 twice); it now takes the first #N that is free - a vertex that already belongs to a graph, this one or another, could be added: it was then listed by both. It now raises ValueError; a removed vertex can be added again, or elsewhere - every check precedes any change, so a refused add leaves the graph and the vertex exactly as they were Co-Authored-By: Claude Sonnet 5.5 --- src/pgraph/PGraph.py | 27 +++++++- tests/test_add_vertex_names.py | 116 +++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 tests/test_add_vertex_names.py diff --git a/src/pgraph/PGraph.py b/src/pgraph/PGraph.py index 77abd3a4..cfc7255d 100644 --- a/src/pgraph/PGraph.py +++ b/src/pgraph/PGraph.py @@ -522,6 +522,8 @@ def add_vertex( :type name: str, optional :raises TypeError: ``coord`` is a ``BaseVertex`` that is not an instance of this graph's :attr:`vertex_class` + :raises ValueError: the name is already used by another vertex in the + graph, or ``coord`` is a vertex that already belongs to a graph :raises ValueError: the graph was constructed with ``dim``, and ``coord`` is given but its length doesn't match :return: the added vertex @@ -535,7 +537,17 @@ def add_vertex( to the graph. If the vertex has no name and ``name`` is None give it a default name - ``#N`` where ``N`` is a consecutive integer. + ``#N`` where ``N`` is the number of vertices, or the next integer above + it that is not already used by a name in the graph. + + Names are unique within a graph, since they are how ``g[name]`` finds a + vertex, and a vertex belongs to one graph at a time. + + .. versionchanged:: 1.0 + Adding a vertex whose name is already used raises ``ValueError``. + It used to replace the earlier vertex in the name table, leaving it + in the graph but unreachable by name. A default name could also + clash with an existing one after a vertex had been removed. The vertex is placed into a dictionary with a key equal to its name. @@ -569,6 +581,11 @@ def add_vertex( """ if isinstance(coord, self._vertex_class): vertex = coord + if vertex._graph is not None: + raise ValueError( + f"vertex {vertex.name!r} already belongs to a graph; " + "remove it from that graph first" + ) elif isinstance(coord, BaseVertex): raise TypeError( f"expecting {self._vertex_class.__name__} or coordinate data, " @@ -590,7 +607,13 @@ def add_vertex( if name is None: name = vertex.name if name is None: - name = f"#{len(self._vertexlist)}" + # the first #N not already used: a removal makes len() reuse a name + n = len(self._vertexlist) + while f"#{n}" in self._vertexdict: + n += 1 + name = f"#{n}" + elif name in self._vertexdict: + raise ValueError(f"the graph already has a vertex named {name!r}") vertex.name = name self._vertexlist.append(vertex) self._vertexdict[vertex.name] = vertex diff --git a/tests/test_add_vertex_names.py b/tests/test_add_vertex_names.py new file mode 100644 index 00000000..59d4c07d --- /dev/null +++ b/tests/test_add_vertex_names.py @@ -0,0 +1,116 @@ +""" +A graph's vertices have unique names (the name is the key ``g[name]`` looks +them up by), and a vertex belongs to at most one graph. ``add_vertex`` used to +overwrite the earlier vertex's table entry on a clash, and the default +``#N`` names could clash after a removal; it now refuses, and chooses a default +name that is free. +""" + +import unittest + +from pgraph import DGraph, UGraph, UVertex + + +class _AddVertex: + Graph: type + Vertex: type + + def setUp(self): + self.g = self.Graph() + + def snapshot(self, g): + return (g.n, sorted(g._vertexdict), [v.name for v in g]) + + def test_duplicate_name_is_refused(self): + a = self.g.add_vertex(name="a") + before = self.snapshot(self.g) + with self.assertRaises(ValueError) as cm: + self.g.add_vertex(name="a") + self.assertIn("'a'", str(cm.exception)) + self.assertEqual(self.snapshot(self.g), before) + self.assertIs(self.g["a"], a) + self.assertEqual(self.g.validate(), []) + + def test_duplicate_through_a_vertex_instance(self): + self.g.add_vertex(name="a") + clash = self.Vertex([1, 1], name="a") + with self.assertRaises(ValueError): + self.g.add_vertex(clash) + self.assertIsNone(clash._graph) # untouched: not half added + self.assertEqual(self.g.n, 1) + + def test_name_argument_overriding_the_instance_name_is_checked_too(self): + self.g.add_vertex(name="taken") + v = self.Vertex([0, 0], name="free") + with self.assertRaises(ValueError): + self.g.add_vertex(v, name="taken") + self.assertEqual(v.name, "free") # a refused add does not rename it + + def test_default_names_are_unique_after_removals(self): + vs = [self.g.add_vertex() for _ in range(4)] # #0 .. #3 + self.g.remove_vertex(vs[0]) + self.g.remove_vertex(vs[2]) + more = [self.g.add_vertex() for _ in range(3)] + names = [v.name for v in self.g] + self.assertEqual(len(names), len(set(names))) + self.assertEqual(self.g.n, 5) + self.assertEqual(self.g.validate(), []) + for v in more: + self.assertIs(self.g[v.name], v) + + def test_default_name_skips_one_given_explicitly(self): + self.g.add_vertex(name="#1") + a = self.g.add_vertex() + b = self.g.add_vertex() + self.assertEqual(len({"#1", a.name, b.name}), 3) + self.assertEqual(self.g.validate(), []) + + def test_default_names_still_count_from_zero(self): + self.assertEqual([self.g.add_vertex().name for _ in range(3)], ["#0", "#1", "#2"]) + + def test_a_vertex_in_this_graph_cannot_be_added_again(self): + a = self.g.add_vertex(name="a") + with self.assertRaises(ValueError): + self.g.add_vertex(a) + self.assertEqual(self.g.n, 1) + self.assertEqual(self.g.validate(), []) + + def test_a_vertex_in_another_graph_cannot_be_added(self): + other = self.Graph() + v = other.add_vertex(name="v") + with self.assertRaises(ValueError): + self.g.add_vertex(v) + self.assertIs(v._graph, other) # still where it was + self.assertEqual(other.validate(), []) + self.assertEqual(self.g.n, 0) + + def test_a_removed_vertex_can_be_added_again_or_elsewhere(self): + a = self.g.add_vertex(name="a") + self.g.remove_vertex(a) + self.assertIs(self.g.add_vertex(a), a) + self.g.remove_vertex(a) + other = self.Graph() + self.assertIs(other.add_vertex(a), a) + self.assertEqual(other.validate(), []) + + def test_wrong_dimension_changes_nothing(self): + g = self.Graph(dim=3) + v = self.Vertex([1, 2], name="short") + with self.assertRaises(ValueError): + g.add_vertex(v) + self.assertEqual(g.n, 0) + self.assertIsNone(v._graph) + + +class TestUGraph(_AddVertex, unittest.TestCase): + Graph, Vertex = UGraph, UVertex + + +class TestDGraph(_AddVertex, unittest.TestCase): + from pgraph import DVertex as Vertex + + Graph = DGraph + + +if __name__ == "__main__": + unittest.main() From 8cab999d3f7ce515310f1ec017a2acf61242d535 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Mon, 12 Oct 2026 09:24:11 +1000 Subject: [PATCH 7/7] test: mark the seeded fuzzing generators as not security use Co-Authored-By: Claude Sonnet 5.5 --- tests/test_in_edges.py | 2 +- tests/test_validate.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_in_edges.py b/tests/test_in_edges.py index 08395b95..3ae807ff 100644 --- a/tests/test_in_edges.py +++ b/tests/test_in_edges.py @@ -175,7 +175,7 @@ def test_removing_a_vertex_clears_edges_in_both_directions(self): self.assertEqual(c.out_degree, 0) def test_random_sequences(self): - rng = random.Random(7) + rng = random.Random(7) # nosec B311 - seeded test fuzzing, not security for trial in range(25): g = DGraph() vs = [g.add_vertex([i, 0]) for i in range(8)] diff --git a/tests/test_validate.py b/tests/test_validate.py index caa8dae5..7c721e6f 100644 --- a/tests/test_validate.py +++ b/tests/test_validate.py @@ -63,7 +63,7 @@ def test_stays_valid_through_removals(self): self.assertEqual(g.validate(), []) def test_random_edit_sequences(self): - rng = random.Random(3) + rng = random.Random(3) # nosec B311 - seeded test fuzzing, not security for Graph in (UGraph, DGraph): g = Graph() vs = [g.add_vertex([i, 0]) for i in range(7)]