Skip to content

[Critical] Make graph semantics explicit: simple vs multi graph types - #1194

Open
isPANN wants to merge 11 commits into
mainfrom
fix/simple-graph-invariant
Open

isPANN wants to merge 11 commits into
mainfrom
fix/simple-graph-invariant

Conversation

@isPANN

@isPANN isPANN commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Graph semantics now live in the type. A problem that allows self-loops or parallel edges uses a multi type and says so at the start of its definition.

Graph types. The naming rule is <Simple|Multi><Graph|Digraph|MixedGraph>.

Type Meaning Change
SimpleGraph undirected; no self-loops; no repeated edges ((u,v) and (v,u) count as the same edge) now rejected in try_new, new and serde
SimpleDigraph directed; no self-loops; no parallel arcs ((u,v) and (v,u) are distinct arcs) new: a thin wrapper around MultiDigraph
MultiDigraph directed; self-loops and parallel arcs allowed; arc i is the i-th entry of arcs() renamed from DirectedGraph; has_arc replaced by arc_count
MultiMixedGraph mixed; self-loops and parallels allowed renamed from MixedGraph
  • Shared validation. One loop/duplicate checker serves both simple types. PlanarGraph and BipartiteGraph reuse it. 21 per-model self-loop checks are removed.
  • Models on SimpleDigraph: AcyclicPartition, DirectedHamiltonianPath, Kernel, MinimumDummyActivitiesPert, MultipleChoiceBranching, StrongConnectivityAugmentation.
  • Models on MultiDigraph:
    • the 6 integral and cost flow models;
    • MinimumFeedbackArcSet and MinimumFeedbackVertexSet;
    • PathConstrainedNetworkFlow;
    • EulerianPath and MinimumCostCirculation.
  • Models on MultiMixedGraph: MixedChinesePostman. CLI/MCP create now accept its loops too.
  • Paper. Multi-typed definitions open with "possibly containing self-loops and parallel arcs". TravelingSalesman and BottleneckTravelingSalesman are defined on simple graphs.

Reductions that used to emit loops or duplicate edges now emit simple graphs:

Reduction Change Formulas
SAT→MIS, SAT→DominatingSet remove duplicate edges unchanged
NAE-SAT→MaxCut add parallel weights together; omit loops, which never affect a cut unchanged
CircuitSAT→SpinGlass move diagonal terms into the constant energy offset unchanged
SAT→Coloring remove duplicate edges; an empty clause becomes a K4 gadget vertex bound gains num_clauses; edge bound becomes 3·num_clauses
3-SAT→DecisionVertexCover an empty clause gives bound −1 num_edges becomes an upper bound
  • Complement formulas stay exact. MIS↔Clique and MinimumMaximalMatching→AchromaticNumber keep num_edges = n(n−1)/2 − m, which is now sound because inputs are simple.
  • TSP→ILP fix. For n ≤ 2 the old ILP had a solution although no Hamiltonian cycle exists; at n = 2 it allowed one edge used twice. The rule now emits 0 = 1. Extraction also rejects infeasible assignments.
    • For n ≥ 3, correctness is proved (the position permutation rules out subtours, and exact McCormick rows make the cost correct for weights of either sign) and checked exhaustively against BruteForce and HiGHS on every simple graph with up to 5 vertices and signed weights.
    • The related rules BTSP→ILP, TSP→QUBO, HC→TSP and HC→BTSP were checked the same way and are fine.

Test speed.

  • The library test binary drops from 58.6 s on main to 19.8 s. Every test that took 1 s or more is now at most 0.25 s, including one 6.9 s test that broke the 5 s limit. No test was deleted for speed.
  • Scope that got narrower:
    • QUBO overhead tests verify energy and slack certificates instead of re-optimizing the QUBO;
    • the clique-cover closed loop checks one cover certificate;
    • the unit-disk map-back, compression and triangular fixtures are smaller;
    • Tutte: branch-and-bound no longer re-solves the full 46-vertex optimum. The test validates the stored 46-vertex order (width 6) and runs exact search on a 14-vertex induced subgraph against an independent DP.

Tests removed because their inputs are now invalid. These 13 tests exercised loops or repeated edges on simple-graph models:

  • test_self_loops_consume_cover_budget
  • polynomial_encoding_preserves_parallel_edge_costs_and_self_loops
  • test_kcoloring_to_bicliquecover_native_loops_are_infeasible
  • test_kcoloring_to_bicliquecover_repeated_reversed_edges
  • test_kcoloring_to_tdcs_native_loops_are_no
  • test_domatic_normalization_ignores_parallel_edges_and_loops
  • test_reduction_shape_with_repeated_edges
  • test_minimumvertexcover_to_ensemblecomputation_loops_and_parallel_edges
  • matching_partition_ignores_loops_and_repeated_adjacencies
  • test_single_vertex_candidate_selection_must_still_respect_budget
  • test_tours_with_parallel_edges_and_loops
  • sink_self_loop_cannot_supply_net_flow
  • sink_self_loop_cannot_supply_either_commodity

Review fixes.

  • CLI, MCP and model create paths now call try_new, so malformed graph input returns a construction error (exit 1) instead of panicking (exit 101). That covers 9 call sites, including two BipartiteGraph panics found during the audit.
  • The decision-search minimization i64 boundary cases are restored with FixedObjective.
  • A lost cheap spanning-tree case is restored.

Breaking change. The public DirectedGraph and MixedGraph are removed, and several model constructors now take SimpleDigraph. The next release therefore needs a minor version bump.

Test plan

  • Rejection tests for self-loops, repeated edges and reversed repeats, through construction, serde, CLI and MCP
  • SimpleDigraph rejects loops and parallel arcs and accepts (u,v) together with (v,u); the multi types accept both
  • Exhaustive TSP→ILP differential test (all 53 graph classes up to n = 5, four weight patterns)
  • All 92 JSON fixtures scanned: no invalid simple-graph records
  • CLI and MCP: malformed graphs give a normal error without panicking (HamiltonianCircuit 0-0 and 0-1,1-0, MaximumDomaticNumber 0-1,1-0 all exit 1)
  • make fmt, make clippy, make check (7,059 tests), make cli, make paper
  • Coverage: no previously covered line lost; the new topology types are at 100%

🤖 Generated with Claude Code

isPANN and others added 9 commits October 11, 2026 23:18
Reject self-loops and repeated undirected edges in SimpleGraph::try_new,
shared by new and serde. Reuse validation for bipartite pairs and propagate
fallible graph construction through model create specs.

Keep complement-edge formulas exact. Make SAT graph builders emit unique
pairs, combine MaxCut contributions, fold constant SpinGlass diagonals into
the decision bound, and use simple NO gadgets for empty clauses.

Update definitions and graph-type documentation, restrict tests to the
accepted simple domain, and verify topology, CLI, MCP, and catalogue create
paths. Keep the standalone macro attribute documentation as text for doctests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Describe TSP and bottleneck TSP as simple-graph problems and remove
obsolete degenerate-tour explanations from their reduction proofs.

Review the other loop and parallel-arc definition additions. Retain
explicit semantics for flow, feedback-set, and mixed postman inputs;
restore simple input definitions for the six catalog formulations whose
broader constructor acceptance still needs a maintainer decision.

Leave model validation and reduction behavior unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rename the loop- and parallel-record topologies to MultiDirectedGraph and
MultiMixedGraph. Add SimpleDirectedGraph as a validated, JSON-compatible
wrapper and migrate the six simple-digraph models and their constructors.

Expose arc multiplicity through MultiDirectedGraph::arc_count, keep per-arc
vector order, and share the existing loop/duplicate checker across simple
topologies. Validate augmentation candidates with that checker and allow
undirected loops through MixedChinesePostman CLI/MCP creation.

Adapt four obsolete simple-model fixtures, retain exhaustive Kernel/ILP
verification over every three-vertex simple digraph, and update graph docs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nches

Test multiplicity, incidence, equality, serde validation, and weighted loop
creation through the CLI and real MCP protocol. Remove unreachable TSP
multigraph recovery and undirected-flow self-loop handling. Drop the
redundant NAE edge-sum overflow branch after its checked total bound.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Share equivalent create-path graph validation across eleven models and
remove checks that simple graph invariants make redundant. Reuse borrowed
arc iteration, avoid neighbor-vector allocations, and build incoming
branching constraints in one pass. Remove obsolete loop-support text.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Emit an infeasible ILP for graphs with fewer than three vertices and require target feasibility before extracting a tour. Preserve the exact variable count and adjust the constraint bound for the contradictory row.

Add exhaustive graph-isomorphism differential tests through five vertices, signed and zero weights, invalid-witness rejection, and exact boundary-cost checks. Document the correctness and overhead arguments in the paper.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep every TSP graph and weight case while checking exact assignments directly, reusing source tours and precomputing vertex relabelings. Reuse the parameter fixture database and target measurements, and prefer smaller authored rule sources over display-model examples.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Retain every test and named graph case. Use exact ILP infeasibility checks for the adversarial reductions, mathematical lower bounds for expensive canonical examples, and verified QUBO slack/energy certificates from optimized ILP prefixes.

Keep exhaustive rooted-tree graph and threshold coverage while pruning invalid reference permutations. Bound the large mapping, determinism and compression fixtures, and verify the full Tutte ordering alongside an independent small exact-search oracle. Remove the two obsolete Tutte ignore attributes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.43820% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.01%. Comparing base (f11b2a4) to head (878b10d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/unit_tests/parameter_formula_validation.rs 97.93% 3 Missing ⚠️
src/rules/travelingsalesman_ilp.rs 94.59% 2 Missing ⚠️
src/unit_tests/rules/travelingsalesman_ilp.rs 99.00% 2 Missing ⚠️
...ules/ksatisfiability_decisionminimumvertexcover.rs 87.50% 1 Missing ⚠️
src/unit_tests/example_db.rs 97.72% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1194      +/-   ##
==========================================
+ Coverage   96.90%   97.01%   +0.11%     
==========================================
  Files        1095     1097       +2     
  Lines      145109   145313     +204     
==========================================
+ Hits       140621   140979     +358     
+ Misses       4488     4334     -154     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@isPANN

isPANN commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

Agentic Review Report

Recommendation: request changes. One Critical finding, one Important finding, and one Minor finding. Reviewed head 6ce204fd43361f2acabe36a2aa88b1fa83df2e62 against merge base 875b6e86. No code was modified, committed, or pushed.

Structural and semantic pass

  • Generic PR; 186 changed paths screened. No blacklisted generated exports. No linked issue or human review comments were present when scope was gathered.
  • No construction bypass found for SimpleGraph or SimpleDigraph: private storage, constructors/builders, serde, and registered model constructors preserve the invariant. The remaining CLI failure concerns error propagation, not acceptance of an invalid graph.
  • The simple/multi model assignments agree with the repository's revised paper definitions. Audited multi-model paths retain arc indices and multiplicity; parallel-path flow, feedback-arc removal, circulation, Eulerian traversal, and mixed-postman checks produced the expected results.
  • The five requested reductions and their formulas are sound on the traced cases: K4 rejects an empty SAT clause; the empty 3-SAT clause gets bound −1; summed NAE weights preserve occurrence contributions; removing a diagonal SpinGlass term subtracts the same constant from the decision bound; TSP with fewer than three vertices produces 0 = 1. The TSP parameter bounds also cover that early return.
  • DirectedGraph and MixedGraph were public exports, so their removal is breaking. has_arc removal and changed model constructor types add migration work. This is acceptable under .claude/skills/release/SKILL.md:33 only in a pre-1.0 minor release, not a 0.6.x patch. Aliases alone would not preserve constructors that now require SimpleDigraph.

Quality pass

  • cargo test --workspace --all-features -- --test-threads=2: 7,113 passed, 3 doctests ignored. The library suite took 19.72 seconds.
  • Formatting and cargo clippy --workspace --all-features --all-targets -- -D warnings passed. All PR CI checks were green at completion.
  • Every build used CARGO_BUILD_JOBS=2; test threads were limited to two, with one build workflow at a time.
  • QUBO certificate checks and smaller mapping fixtures are disclosed. Removed loop/parallel-edge tests concern inputs that are now invalid. Two additional coverage/disclosure problems are listed below.

pred feature-test pass

Installed the PR CLI using make cli and exercised it from a scratch directory. Ran list/show, example creation, inspect, direct/default and brute-force solve, measured path discovery, explicit-route reduction, and bundle solve for SAT→Coloring, 3-SAT→DecisionVertexCover, NAE-SAT→DecisionMaxCut, CircuitSAT→DecisionSpinGlass, TSP→ILP, and EulerianPath→ILP. Recovered evaluations agreed with direct solves; weighted TSP returned Min(80).

Additional checks confirmed empty SAT is infeasible, repeated/complementary NAE literals behave correctly, repeated AND/OR/XOR inputs preserve circuit solutions, and TSP at n=0,1,2 is infeasible. The allow-less empty 3-SAT case passed its library test; the persisted K3 schema rejects short clauses. Independent parallel/loop examples returned feedback-arc cost 8, circulation cost −9, and mixed-postman cost 17. Real MCP transport rejected invalid HamiltonianCircuit graphs while keeping the server alive and evaluated the mixed parallel-loop example as Min(17).

Findings, sorted by severity

  1. Critical — malformed graph input panics through the shared CLI parser.
    Locations: src/topology/graph.rs:147; caller problemreductions-cli/src/commands/create/schema_support.rs:1124.

    The new invariant makes SimpleGraph::new panic, but the schema parser still calls it at the user-input boundary. Reproduced with the installed PR binary:

    pred create HamiltonianCircuit --graph 0-0 --num-vertices 3
    pred create HamiltonianCircuit --graph 0-1,1-0 --num-vertices 3
    pred create MaximumDomaticNumber --graph 0-1,1-0 --num-vertices 3

    Each exits 101 with thread 'main' ... panicked, rather than returning the normal construction error and usage guidance. MaximumIndependentSet rejects the same inputs normally with exit 1, because its CreateSpec uses the fallible path. The new rejection tests cover that path and miss the shared parser.

    Suggested fix: use SimpleGraph::try_new(num_vertices, edges)? in the shared parser and add a rejection test for a model using the ordinary schema path, asserting a normal error and no panic.

  2. Important — minimization no longer tests the positive i64 boundaries.
    Location: src/unit_tests/solvers/decision_search.rs:70–74.

    Replacing the forced loop with an isolated vertex changes the minimum from weight to min(weight, 0). Thus the i64::MAX and i64::MAX - 1 cases now both test optimum zero; I reproduced Min(0) for the replacement MAX-weight instance. Maximization uses a separate search implementation and cannot protect minimization here. A regression returning MAX−1 or infeasibility when a minimization optimum is MAX would escape these tests. This lost property is absent from the PR's scope-narrowing list.

    Suggested fix: reuse the existing FixedObjective(Min(Some(value))) model to retain every integer-boundary case and the singleton intervals, while keeping graph fixtures valid.

  3. Minor — full Tutte optimization coverage was replaced but not disclosed.
    Location: src/unit_tests/rules/unitdiskmapping/pathdecomposition.rs:426–450.

    The former ignored test ran branch-and-bound on all 46 Tutte vertices and checked optimum 6. It now validates the stored full-graph order and searches only a 14-vertex induced subgraph. A full-Tutte optimizer regression returning width 7 would pass the replacement. The PR lists smaller mapping fixtures, but omits this distinct optimizer-coverage change; its “No test was deleted” claim also overlooks removed invalid-domain test functions.

    Suggested fix: explicitly document the lost full-Tutte optimum check and the invalid-domain test removals; describe the replacement as certificate validation plus bounded exact-search coverage.

Coverage collection, paper rendering, and an independent audit of upstream literature were not rerun. The reproduced CLI panic remains a merge risk despite green tests and CI.

isPANN and others added 2 commits October 12, 2026 09:40
Use fallible topology constructors at schema and typed create boundaries,
including bipartite models, and check vertex-count inference for overflow.
Exercise ordinary schema models and typed MCP inputs without process panics.

Restore positive minimization boundary and singleton-interval checks using
the fixed-objective fixture, and retain the cheap spanning-tree budget case
that was lost when its parallel edge fixture became a simple path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reuse the existing permutation decoder and Cartesian iterator, consolidate
fixture and MCP assertion loops, and stream the rooted-tree oracle costs.
Avoid temporary TSP edge triples and simplify equivalent solver budgets.
Keep every test and case, typed create errors, and independent certificate
checks intact.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@isPANN isPANN changed the title Make graph semantics explicit: simple vs multi graph types [Critical] Make graph semantics explicit: simple vs multi graph types Oct 17, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant