Graph-layer architecture review: model/renderer split, correctness + perf fixes, and issue #1 null-flow routing switch - #79
Merged
marota merged 10 commits intoJul 16, 2026
Conversation
Implements code-review recommendations (see docs/CODE_REVIEW.md). Correctness - rename_nodes: fix idx_ex column rebuilt from idx_or (copy-paste bug) - OverFlowGraph.__init__ copies the caller DataFrame (no more in-place mutation) - find_loops: re-enable the simple-path cutoff (configurable loop_path_cutoff, default 10 nodes) to avoid hanging on large grids; add a consolidation-path cutoff (default 20 edges) - null_flow_graph._compute_sssp_paths: narrow bare except to (nx.NetworkXException, ValueError) and log a warning instead of swallowing Performance - delete_color_edges accepts a single colour or an iterable; single graph copy - Structured_Overload_Distribution_Graph builds each derived colour view in one pass (fewer full-graph copies) - alphadeesp ranking: vectorise sort_hubs (group-sums) and precompute an inflow lookup for rank_loop_buses (keeps _initial_inflow_between as fallback) Deep revision - Split OverFlowGraph into a semantic model (OverFlowGraph) and a stateless Graphviz renderer (OverflowGraphRenderer); the facade delegates all presentation (penwidth, shapes, tapered styling, highlight labels/colours, plotting). Public signatures unchanged; renderer exported from the package and the back-compat shim. Docs & tests - docs/CODE_REVIEW.md (review + what-was-implemented tracker); CLAUDE.md refresh - new tests for rename_nodes, df-copy, sort_hubs, inflow lookup, and the standalone renderer; graphs-package contract test updated for the new symbol Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
…t review Follows an adversarial multi-agent verification of the previous commit. - find_loops / consolidation path cutoffs are now OPT-IN (default None = unbounded = original behaviour). Enabling cutoff=10 by default was itself a regression: rustworkx cutoff counts nodes and real RTE zone grids have loop paths of 15-42 nodes, so the default silently dropped legitimate loops and could empty find_loops. - Guard the empty-loops case in get_dispatch_edges_nodes: an empty Path column sum() returns scalar 0, breaking set(0); return [] instead (regression test added). - Correct the review record: the rename_nodes finding was a misread — the original iterated the correct idx_ex column via a misleadingly-named loop variable, so it was functionally correct (not a data-corruption bug). The change is a readability improvement only. docs/CODE_REVIEW.md and CLAUDE.md updated accordingly. All 342 runnable (non-grid2op) unit tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
Cross-repo audit of the graph-layer changes against the marota fork of Expert_op4grid_recommender: no breaking changes, one strictly-beneficial behavioural change (the get_dispatch_edges_nodes empty-loops guard removes a latent TypeError the recommender's non-antenna orchestrator branch could hit). All other vectors (df.copy, model/renderer split, cutoffs default None, delete_color_edges union, warmStart, import surface) are benign/unobservable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
Feuille de route des pistes restantes (open findings ainetus#4/ainetus#6/ainetus#7, CI static-analysis repoint, mixins->composition, interactive_html/Grid2opSimulation decomposition, model-vs-colour inversion, AlphaDeesp run() API, naming aliases, grid2op CI validation + release/bump). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
…aph, create_df, shortest paths, interactive_html split Deep revisions: - Model/colour inversion: new graphs/edge_roles.py (edge_role_of / base_color_of / EDGE_ROLE_*). tag_constrained_path reads the semantic role instead of parsing the Graphviz colour string; highlight_significant_line_loading records an authoritative base_color when it wraps a colour into the compound "c:yellow:c"; new OverFlowGraph.edge_role(name). Exported via the package + shim. - AlphaDeesp.__init__ -> explicit run(): the ranking pipeline moved into run(); auto_run=True (default) preserves backwards-compatible construction; results are initialised empty so auto_run=False builds a well-formed, un-run object. - Structured_Overload_Distribution_Graph: colour-filtered views, red_loops and hubs are now functools.cached_property (constrained path stays eager). red_loops uses the constructor seed hubs; public find_loops() uses the detected hubs — this split makes the lazy properties order-independent while matching the old eager behaviour exactly. Open findings: - ainetus#4 create_df: vectorised the iterrows passes with numpy masks; ltc_report now uses positional .iloc. Behaviour pinned by a 400-case differential fuzz against a faithful re-implementation of the original loop. - ainetus#6 shortest_paths.py: shared _make_incentivized_weight, multigraph-correct (min parallel weight + (u,v)/(u,v,key) promoted matching); dead branch removed. - ainetus#7 to_DiGraph: a missing capacity now defaults to 0.0 (neutral in the min-cut) instead of a spurious 1.0. Maintainability: - interactive_html.py (976 LOC) -> package interactive_html/ (8 focused modules) with the CSS/JS/HTML skeleton externalised under assets/ and reassembled byte-exactly at runtime; shipped via package_data + MANIFEST.in. Docs: CLAUDE.md and docs/CODE_REVIEW.md updated. All graph-package / ranking / renderer / create_df / interactive-html unit suites pass (367) without grid2op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
Adversarial verification surfaced a latent equivalence break in the lazy Structured_Overload_Distribution_Graph refactor: the colour-filtered views were computed lazily from ``g_init``, a *live* reference to the caller's graph. The original eager __init__ copied every view at construction, so a later mutation of that shared graph (consolidate_graph removes the ignored lines from the same OverFlowGraph.g before re-reading this object) did not leak into the views. Deferring the copies to first access read the mutated graph instead. Fix: snapshot the graph once at construction (self._g_snapshot = g.copy()) and compute the lazy views from that frozen snapshot — exact construction-time snapshot semantics, views stay lazy. g_init stays a live reference because get_constrained_edges_nodes historically read names off the live graph. Regression test reproduces the scenario (mutate the caller graph after construction; views must still reflect the snapshot). Production callers were already protected (empty ignore-list + pre-access), but this restores exact equivalence for the general/consolidation path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
…(issue #1) Issue #1: on the overflow MultiDiGraph, networkx hands a callable weight the {key: attr} parallel-edge view, so `_compute_sssp_paths`'s `attr.get("capacity",0)` silently read 0 and the (u,v) promoted test never matched the (u,v,key) set — routing was a uniform hop cost, not capacity-weighted. Change (keeps both options, per maintainer decision): - Materialise the routing weight once as an edge attribute and run Dijkstra with a string weight instead of a per-edge-relaxation Python callable (the dominant non-load-flow cost at national scale). - capacity_weighted=False (default, "bless"): reproduce the historical effective behaviour exactly (uniform hop weight) — bit-identical, just explicit + fast. - capacity_weighted=True ("fix"): capacity-weighted routing with correct multigraph min-parallel capacity and (u,v)/(u,v,key) promoted matching. - Flag threaded through add_relevant_null_flow_lines[_all_paths] so downstream callers (Expert_op4grid_recommender) can switch modes without patching. Tests: bit-identical differential vs the old callable; Option A hop-only vs Option B capacity-weighted on the issue's repro; promoted (u,v,key) matching. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
Bump version 0.3.2.post4 -> 0.3.3 (setup.py, docs/conf.py, download_url) and add a 0.3.3 CHANGELOG entry summarising the graph-layer architecture-review pass: model/renderer split, edge-role accessor, AlphaDeesp run(), lazy structured graph, create_df/ranking vectorisation, shortest_paths + null-flow multigraph Dijkstra-weight fix (issue #1, capacity_weighted switch), and the interactive_html package split with externalised assets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
marota
force-pushed
the
claude/repo-architecture-review-m4rhkh
branch
from
July 16, 2026 13:23
943c3e7 to
4961461
Compare
Add dedicated suites for the modules the coverage report flagged as weakest among the unit-testable code (no grid2op needed): - test_graph_consolidation.py: GraphConsolidationMixin (reverse_edges, consolidate_loop_path, reverse_blue_edges_in_looppaths, _is_ambiguous_component, desambiguation_type_path) — coverage 24% -> 62%. - test_elements.py: Production/Consumption/OriginLine/ExtremityLine value objects. - test_simulation_helpers.py: Simulation.get_model_obj_from_or/ext MultiIndex lookups (incl. duplicate + swapped branches), invert_dict_keys_values, create_end_result_empty_dataframe. - test_topo_applicator.py: apply_new_topo_to_graph busbar-split graph mutation. - test_alphadeesp_combinations.py: compute_all_combinations, legal_comb, clean_and_sort_best_topologies, filter_constrained_path. 418 runnable tests pass (+46). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
…n docs The repository had no developer/architecture documentation and no API reference (Sphinx built only the hand-written RST, with autodoc disabled). - docs/ARCHITECTURE.rst: developer map of the package — pipeline, the Simulation backend contract, the graphs package (model/renderer split, semantic edge roles, structured overload graph, null-flow + consolidation), AlphaDeesp run()/auto_run, the interactive viewer, and extension points. - docs/API.rst + conf.py: enable sphinx.ext.autodoc + napoleon + viewcode (with autodoc_mock_imports for the heavy optional backends) and generate an API reference from the docstrings; put the repo root on sys.path. - Fix three docstrings in structured_overload_graph.py that autodoc surfaced as reStructuredText errors (find_constrained_path, get_constrained_edges_nodes, get_dispatch_edges_nodes) — now valid NumPy-style. - DETAILS.rst: correct the outdated AlphaDeesp(...) call signature (dropped the long-gone custom_layout/printer args; document auto_run/run()). - index.rst: add a "Developer documentation" toctree. Sphinx `-b html` builds cleanly (API + Architecture pages render the docstrings). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XzsGr6toJXzsLdoKqM5KJ Signed-off-by: Antoine Marot <amarot91@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A large architecture-review pass over the
alphaDeespgraph layer: correctnessfixes, performance work, two deep revisions, the interactive-viewer
decomposition, and the fix for issue #1 (multigraph Dijkstra weight). All
changes are unit-tested (372 runnable tests pass without grid2op) and were
checked by adversarial multi-agent verification.
Highlights
Correctness
rename_nodesclarified;OverFlowGraph.__init__no longer mutates the caller's DataFrame.find_loopspath cutoff made opt-in (Nonedefault = original unbounded); empty-loops guard inget_dispatch_edges_nodes.except;to_DiGraphmissing capacity →0.0(was1.0).create_dfvectorised (400-case differential fuzz vs the original loop) + explicit positional index.shortest_paths.py: dead branch removed; multigraph-correct weight +(u,v)/(u,v,key)promoted matching.Deep revisions
OverflowGraphRendererowns all Graphviz presentation;OverFlowGraphis the semantic model. Newedge_roles.py(edge_role_of) so no consumer parses colour strings;highlight_*records an authoritativebase_color.AlphaDeespexplicitrun()(auto_run=Truedefault, backwards-compatible).red_loops/hubsarecached_property, frozen to a construction-time snapshot (behaviour-identical, order-independent).Maintainability
interactive_html.py(976 LOC) → package (8 focused modules) + externalisedassets/{viewer.css,viewer.js,template.html}, reassembled byte-exactly at runtime.Issue #1 — multigraph Dijkstra weight (null-flow path search)
MultiDiGraph, networkx hands a callable weight the{key: attr}parallel-edge view, so_compute_sssp_paths'sattr.get("capacity", 0)read0— routing was hop-cost-only.capacity_weightedswitch:False(default, bless): reproduces today's effective behaviour bit-identical.True(fix): capacity-weighted routing (correct min-parallel capacity + promoted matching).add_relevant_null_flow_lines[_all_paths]soExpert_op4grid_recommendercan switch modes without patching. Addresses Bug: edge capacity silently ignored by Dijkstra weight callables on MultiDiGraph (effective weight = hop cost only) + validated perf fix for _compute_sssp_paths marota/ExpertOp4Grid_marota#1 pending reference-case validation of the capacity-weighted mode.Docs
docs/CODE_REVIEW.md(full review + downstream-impact analysis onExpert_op4grid_recommender+ remaining-work backlog);CLAUDE.mdrefreshed.Validation
372 runnable unit tests pass (graph package, ranking, renderer, interactive
viewer). The grid2op integration suites and the capacity-weighted routing mode
still want validation on reference grids in CI.
🤖 Generated with Claude Code