diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index eddb4f9..d4001e7 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -16,4 +16,4 @@ ## Bug Fixes - +- Fixed component graph rendering so children follow the vertical order of their parents, keeping upper-level branches above lower ones in the layered layout. \ No newline at end of file diff --git a/src/frequenz/gridpool/cli/_render_graph.py b/src/frequenz/gridpool/cli/_render_graph.py index a47089d..2fe8851 100644 --- a/src/frequenz/gridpool/cli/_render_graph.py +++ b/src/frequenz/gridpool/cli/_render_graph.py @@ -124,7 +124,8 @@ def compute_layout(self, graph: Any) -> dict[Any, tuple[float, float]]: root_node = self._select_root(graph) layered_nodes = self._group_by_level(graph, root_node) - return self._build_positions(layered_nodes) + ordered_layers = self._order_layers(graph, layered_nodes) + return self._build_positions(ordered_layers) def _select_root(self, graph: Any) -> Any: """Select a root node for layout. @@ -171,6 +172,56 @@ def _group_by_level(self, graph: Any, root_node: Any) -> dict[int, list[Any]]: layered_nodes[orphan_layer] = orphans return layered_nodes + def _order_layers( + self, graph: Any, layered_nodes: dict[int, list[Any]] + ) -> dict[int, list[Any]]: + """Order nodes within each layer based on their parent positions. + + Reorders the nodes in each layer to improve visual readability when rendering a + layered graph layout. The first layer is ordered deterministically (by string + representation). For subsequent layers, nodes are ordered primarily by the + lowest index of any already-ordered parent (predecessor) from the previous + layers, with a deterministic string-based tie-breaker. + + Args: + graph: + Graph-like object providing predecessor relationships via + ``graph.predecessors(node)``. + layered_nodes: + Mapping from layer index to the list of nodes assigned to that layer. + + Returns: + A new mapping with the same layer keys as ``layered_nodes``, where each + layer's node list is ordered to align children beneath their parents. + """ + ordered_layers: dict[int, list[Any]] = {} + previous_order: dict[Any, int] = {} + + for level in sorted(layered_nodes): + nodes = layered_nodes[level] + if level == 0: + ordered_nodes = sorted(nodes, key=str) + else: + + def sort_key(node: Any) -> tuple[int, str]: + parents = [ + parent + for parent in graph.predecessors(node) + if parent in previous_order + ] + if parents: + parent_index = min(previous_order[parent] for parent in parents) + else: + parent_index = len(previous_order) + return (parent_index, str(node)) + + ordered_nodes = sorted(nodes, key=sort_key) + + ordered_layers[level] = ordered_nodes + previous_order = {node: idx for idx, node in enumerate(ordered_nodes)} + + return ordered_layers + def _build_positions( self, layered_nodes: dict[int, list[Any]] ) -> dict[Any, tuple[float, float]]: @@ -187,11 +238,11 @@ def _build_positions( """ x_spacing, y_spacing = 2.5, 1.2 pos: dict[Any, tuple[float, float]] = {} - for level, nodes in layered_nodes.items(): + for level in sorted(layered_nodes): + nodes = layered_nodes[level] x_pos = level * x_spacing - sorted_nodes = sorted(nodes, key=str) - y_start = (len(sorted_nodes) - 1) * y_spacing / 2 - for i, node in enumerate(sorted_nodes): + y_start = (len(nodes) - 1) * y_spacing / 2 + for i, node in enumerate(nodes): pos[node] = (x_pos, y_start - (i * y_spacing)) return pos diff --git a/tests/test_render_graph.py b/tests/test_render_graph.py index fca10f1..5c2d6eb 100644 --- a/tests/test_render_graph.py +++ b/tests/test_render_graph.py @@ -122,7 +122,7 @@ def test_group_by_level_adds_orphans() -> None: def test_build_positions_centers_nodes_in_layer() -> None: """It should center nodes vertically within each layer.""" renderer = ComponentGraphRenderer(MagicMock(spec=AssetsApiClient)) - layered_nodes = {0: ["b", "a"], 1: ["c"]} + layered_nodes = {0: ["a", "b"], 1: ["c"]} pos = renderer._build_positions(layered_nodes) # pylint: disable=protected-access @@ -131,6 +131,21 @@ def test_build_positions_centers_nodes_in_layer() -> None: assert pos["c"] == (2.5, 0.0) +def test_compute_layout_orders_children_by_parent_position() -> None: + """It should keep children of higher parents above lower ones.""" + renderer = ComponentGraphRenderer(MagicMock(spec=AssetsApiClient)) + graph: nx.DiGraph[Any] = nx.DiGraph() + graph.add_edge("root", "a") + graph.add_edge("root", "b") + graph.add_edge("a", "a_child") + graph.add_edge("b", "b_child") + + pos = renderer.compute_layout(graph) + + assert pos["a"][1] > pos["b"][1] + assert pos["a_child"][1] > pos["b_child"][1] + + def test_render_writes_file_without_show(monkeypatch: pytest.MonkeyPatch) -> None: """It should save a figure and avoid showing it when configured.""" renderer = ComponentGraphRenderer(MagicMock(spec=AssetsApiClient))