Fix #73: Refactor graph problem constructors to take graph as input#74
Fix #73: Refactor graph problem constructors to take graph as input#74
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
+ Coverage 96.36% 96.38% +0.01%
==========================================
Files 191 190 -1
Lines 25494 25306 -188
==========================================
- Hits 24568 24391 -177
+ Misses 926 915 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… with generic new(graph, weights) Remove the `impl<W> MaximumIndependentSet<SimpleGraph, W>` block containing `new(num_vertices, edges)` and `with_weights(num_vertices, edges, weights)`. Rename `from_graph` to `new` in the generic impl block, making all graph types use the same `MaximumIndependentSet::new(graph, weights)` constructor. Update all call sites across rules, unit tests, integration tests, examples, benchmarks, and doc comments to use the new pattern: - `MaximumIndependentSet::<SimpleGraph, i32>::new(n, edges)` becomes `MaximumIndependentSet::new(SimpleGraph::new(n, edges), vec![1i32; n])` - `MaximumIndependentSet::with_weights(n, edges, w)` becomes `MaximumIndependentSet::new(SimpleGraph::new(n, edges), w)` - `MaximumIndependentSet::from_graph(g, w)` becomes `MaximumIndependentSet::new(g, w)` Part of #73 (graph constructor refactoring). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ue, MaximalIS Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…m_graph -> new - Delete `impl<K: KValue> KColoring<K, SimpleGraph>` block with `new(num_vertices, edges)` - Rename `from_graph(graph)` -> `new(graph)` in generic `impl<K: KValue, G: Graph>` - Rename `from_graph_with_k(graph, k)` -> `with_k(graph, k)` in `impl<G: Graph> KColoring<KN, G>` - Update all call sites: wrap edges in `SimpleGraph::new(n, edges)` and use `KColoring::<K3, _>::new(...)` - Update doc example, rules, unit tests, integration tests, examples, and benchmarks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tching, TravelingSalesman Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… vertices/edges Update 8 public helper functions (is_independent_set, is_vertex_cover, is_clique, is_maximal_independent_set, is_dominating_set, is_matching, is_hamiltonian_cycle, is_valid_coloring) to accept `&G where G: Graph` instead of `(num_vertices, edges)`. Size mismatches now panic via assert_eq! instead of returning false. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors graph problem constructors to accept an explicit graph instance (making topology types visible at the call site), updates related helpers and call sites, and switches is_weighted() to a type-based check via WeightElement::IS_UNIT.
Changes:
- Removed
new(num_vertices, edges)/unweighted(...)-style constructors across graph problems; standardized onnew(graph, ...)plus renamed convenience ctors (unit_weights,unweighted,with_k). - Updated public helper functions (
is_independent_set,is_vertex_cover,is_clique, etc.) to accept&impl Graphand assert on length mismatches. - Added
WeightElement::IS_UNITand migratedis_weighted()to be type-based.
Reviewed changes
Copilot reviewed 83 out of 83 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/suites/reductions.rs | Update reduction tests to construct graphs explicitly and pass weights separately. |
| tests/suites/integration.rs | Update integration tests to new graph-first constructors and renamed ctors. |
| src/unit_tests/unitdiskmapping_algorithms/common.rs | Update MIS construction in unitdisk mapping helpers to new signature. |
| src/unit_tests/trait_consistency.rs | Update trait conformance tests to new constructors and edge-weight APIs. |
| src/unit_tests/testing/macros.rs | Replace macro-based tests with manual equivalents due to ctor signature changes. |
| src/unit_tests/solvers/brute_force.rs | Update brute force solver test to new MIS constructor signature. |
| src/unit_tests/rules/travelingsalesman_ilp.rs | Update TSP rule tests to new new(graph, weights) / unit_weights(graph) APIs. |
| src/unit_tests/rules/spinglass_maxcut.rs | Update MaxCut instances to separate graph and weights, and renamed unweighted ctor. |
| src/unit_tests/rules/minimumvertexcover_qubo.rs | Update VC→QUBO rule tests to new VC constructors and helper signatures. |
| src/unit_tests/rules/minimumvertexcover_minimumsetcovering.rs | Update VC→SC rule tests to new constructors and explicit graphs. |
| src/unit_tests/rules/minimumvertexcover_maximumindependentset.rs | Update IS↔VC rule tests to new constructors and graph-first pattern. |
| src/unit_tests/rules/minimumvertexcover_ilp.rs | Update VC→ILP rule tests to new constructors. |
| src/unit_tests/rules/minimumdominatingset_ilp.rs | Update DS→ILP rule tests to new constructors. |
| src/unit_tests/rules/maximummatching_maximumsetpacking.rs | Update Matching→SP rule tests to new constructors and unit_weights. |
| src/unit_tests/rules/maximummatching_ilp.rs | Update Matching→ILP rule tests to new constructors and unit_weights. |
| src/unit_tests/rules/maximumindependentset_triangular.rs | Update MIS→TriangularSubgraph rule tests to new MIS constructor. |
| src/unit_tests/rules/maximumindependentset_qubo.rs | Update MIS→QUBO rule tests to new constructors. |
| src/unit_tests/rules/maximumindependentset_maximumsetpacking.rs | Update MIS→SP rule tests to new constructors. |
| src/unit_tests/rules/maximumindependentset_ilp.rs | Update MIS→ILP rule tests to new constructors. |
| src/unit_tests/rules/maximumindependentset_gridgraph.rs | Update MIS gridgraph rule tests to renamed ctor and new signature. |
| src/unit_tests/rules/maximumclique_ilp.rs | Update Clique→ILP rule tests to new constructors. |
| src/unit_tests/rules/graph.rs | Update chained reduction tests to new MIS constructor and graph casting ctor rename. |
| src/unit_tests/rules/coloring_qubo.rs | Update KColoring→QUBO rule tests to new KColoring constructor. |
| src/unit_tests/rules/coloring_ilp.rs | Update KColoring→ILP rule tests to new KColoring constructor. |
| src/unit_tests/property.rs | Update property tests to construct graphs explicitly and pass weights. |
| src/unit_tests/models/set/maximum_set_packing.rs | Update SP relationship test to new MIS constructor. |
| src/unit_tests/models/graph/traveling_salesman.rs | Update TSP model tests to new constructors and type-based weighted semantics. |
| src/unit_tests/models/graph/minimum_vertex_cover.rs | Update VC model tests and helper usage to graph-based helper functions. |
| src/unit_tests/models/graph/minimum_dominating_set.rs | Update DS model tests and helper usage to graph-based helper functions. |
| src/unit_tests/models/graph/maximum_matching.rs | Update Matching model tests and helper usage to graph-based helper functions. |
| src/unit_tests/models/graph/maximum_independent_set.rs | Update MIS model tests and helper usage to graph-based helper functions. |
| src/unit_tests/models/graph/maximum_clique.rs | Update Clique model tests and helper usage to graph-based helper functions. |
| src/unit_tests/models/graph/maximal_is.rs | Update MaximalIS model tests and helper usage to graph-based helper functions. |
| src/unit_tests/models/graph/max_cut.rs | Update MaxCut model tests to new constructors and renamed unweighted ctor. |
| src/unit_tests/models/graph/kcoloring.rs | Update KColoring model tests to new constructor and helper signature behavior. |
| src/unit_tests/io.rs | Update IO tests to new MIS constructor signature. |
| src/unit_tests/graph_models.rs | Update aggregated graph model tests to new constructors and helper signatures. |
| src/types.rs | Add WeightElement::IS_UNIT and implement it for i32, f64, and One. |
| src/topology/mod.rs | Update rustdoc example to construct SimpleGraph explicitly and pass weights. |
| src/solvers/ilp/solver.rs | Update ILP solver rustdoc example to new MIS constructor. |
| src/rules/spinglass_maxcut.rs | Update SG→MaxCut construction to new MaxCut new(graph, weights) signature. |
| src/rules/sat_minimumdominatingset.rs | Update SAT→DS construction to new DS new(graph, weights) signature. |
| src/rules/sat_maximumindependentset.rs | Update SAT→MIS construction to new MIS new(graph, weights) signature. |
| src/rules/sat_coloring.rs | Update SAT coloring builder to new KColoring constructor signature. |
| src/rules/mod.rs | Update macro docs example to renamed ctor (from_graph → new). |
| src/rules/minimumvertexcover_maximumindependentset.rs | Update IS↔VC reduction targets to construct graphs explicitly and pass weights. |
| src/rules/maximumindependentset_triangular.rs | Update MIS→TriangularSubgraph reduction to renamed ctor (new). |
| src/rules/maximumindependentset_maximumsetpacking.rs | Update SP→MIS reduction to new MIS ctor with explicit SimpleGraph. |
| src/rules/maximumindependentset_gridgraph.rs | Update MIS gridgraph reductions to renamed ctor (new). |
| src/rules/maximumindependentset_casts.rs | Update MIS variant casts to renamed ctor (new). |
| src/rules/kcoloring_casts.rs | Update KColoring variant cast to renamed ctor (with_k). |
| src/models/graph/traveling_salesman.rs | Remove SimpleGraph-only ctors, rename from_graph → new, rename unit-weight ctor, update helper signature. |
| src/models/graph/minimum_vertex_cover.rs | Remove SimpleGraph-only ctors, rename from_graph → new, update helper signature and panics. |
| src/models/graph/minimum_dominating_set.rs | Remove SimpleGraph-only ctors, rename from_graph → new, update helper signature and panics. |
| src/models/graph/maximum_matching.rs | Remove SimpleGraph-only ctors, rename from_graph → new, rename unit-weight ctor, update helper signature and panics. |
| src/models/graph/maximum_independent_set.rs | Remove SimpleGraph-only ctors, rename from_graph → new, update helper signature and panics. |
| src/models/graph/maximum_clique.rs | Remove SimpleGraph-only ctors, rename from_graph → new, update helper signature and panics. |
| src/models/graph/maximal_is.rs | Remove SimpleGraph-only ctors, rename from_graph → new, update helper signature and panics. |
| src/models/graph/max_cut.rs | Remove SimpleGraph-only ctors, rename from_graph → new, rename unweighted ctor. |
| src/models/graph/kcoloring.rs | Remove SimpleGraph-only ctor, rename from_graph → new, rename from_graph_with_k → with_k, update helper signature and panics. |
| src/lib.rs | Update crate-level rustdoc example to new MIS constructor signature. |
| src/io.rs | Update IO rustdoc example to new MIS constructor signature. |
| examples/reduction_travelingsalesman_to_ilp.rs | Update example to new TSP new(graph, weights) signature. |
| examples/reduction_minimumvertexcover_to_qubo.rs | Update example to new VC constructor signature. |
| examples/reduction_minimumvertexcover_to_minimumsetcovering.rs | Update example to new VC constructor signature. |
| examples/reduction_minimumvertexcover_to_maximumindependentset.rs | Update example to new VC constructor signature. |
| examples/reduction_minimumvertexcover_to_ilp.rs | Update example to new VC constructor signature. |
| examples/reduction_minimumdominatingset_to_ilp.rs | Update example to new DS constructor signature. |
| examples/reduction_maximummatching_to_maximumsetpacking.rs | Update example to new Matching unit_weights(graph) signature. |
| examples/reduction_maximummatching_to_ilp.rs | Update example to new Matching unit_weights(graph) signature. |
| examples/reduction_maximumindependentset_to_qubo.rs | Update example to new MIS constructor signature. |
| examples/reduction_maximumindependentset_to_minimumvertexcover.rs | Update example to new MIS constructor signature. |
| examples/reduction_maximumindependentset_to_maximumsetpacking.rs | Update example to new MIS constructor signature. |
| examples/reduction_maximumindependentset_to_ilp.rs | Update example to new MIS constructor signature. |
| examples/reduction_maximumclique_to_ilp.rs | Update example to new Clique constructor signature. |
| examples/reduction_maxcut_to_spinglass.rs | Update example to new MaxCut unweighted(graph) signature. |
| examples/reduction_kcoloring_to_qubo.rs | Update example to new KColoring constructor signature. |
| examples/reduction_kcoloring_to_ilp.rs | Update example to new KColoring constructor signature. |
| docs/src/getting-started.md | Update getting-started snippet to explicit graph + weights construction. |
| docs/plans/2026-02-16-graph-constructor-refactoring.md | Add implementation plan documenting the refactor steps and call-site migration. |
| benches/solver_benchmarks.rs | Update benchmarks to new constructors and separated edge weights. |
| .github/ISSUE_TEMPLATE/rule.md | Add “Motivation” field to rule issue template. |
| .github/ISSUE_TEMPLATE/problem.md | Add “Motivation” section to problem issue template. |
Comments suppressed due to low confidence (1)
src/models/graph/maximum_clique.rs:186
is_clique()checks adjacency by callinggraph.has_edge(u, v)for every selected pair. ForSimpleGraph,has_edgeusespetgraph::UnGraph::find_edge, which is not constant-time and can make this O(k^2 * deg) for k selected vertices. To avoid a potential regression, consider building aHashSet(or adjacency bitset) once fromgraph.edges()and then doing O(1) lookups when validating pairs.
// Check all pairs of selected vertices are adjacent
for i in 0..selected_vertices.len() {
for j in (i + 1)..selected_vertices.len() {
if !graph.has_edge(selected_vertices[i], selected_vertices[j]) {
return false;
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Check if a configuration forms a valid Hamiltonian cycle. | ||
| fn is_valid_hamiltonian_cycle(&self, config: &[usize]) -> bool { | ||
| let edges = self.graph.edges(); | ||
| let selected: Vec<bool> = config.iter().map(|&s| s == 1).collect(); | ||
| is_hamiltonian_cycle(self.graph.num_vertices(), &edges, &selected) | ||
| is_hamiltonian_cycle(&self.graph, &selected) | ||
| } |
There was a problem hiding this comment.
TravelingSalesman::evaluate() now indirectly panics when config.len() != graph.num_edges() because is_valid_hamiltonian_cycle() forwards config into is_hamiltonian_cycle(), which asserts on length. Problem::evaluate should generally return SolutionSize::Invalid for malformed assignments rather than panic; consider adding a length check in is_valid_hamiltonian_cycle() (or in evaluate()) and returning false early before calling the asserting helper.
| // Check each vertex is dominated | ||
| for v in 0..num_vertices { | ||
| for v in 0..graph.num_vertices() { | ||
| if selected[v] { | ||
| continue; // v dominates itself | ||
| } | ||
| // Check if any neighbor of v is selected | ||
| let dominated = adj[v].iter().any(|&u| selected[u]); | ||
| let dominated = graph.neighbors(v).iter().any(|&u| selected[u]); | ||
| if !dominated { |
There was a problem hiding this comment.
is_dominating_set() calls graph.neighbors(v) inside the loop over all vertices; the Graph trait returns Vec<usize>, so this allocates on every iteration (O(V) allocations) and can be noticeably slow on larger graphs. Consider building an adjacency list once from graph.edges() (or caching neighbors) and then checking domination using that structure.
- TravelingSalesman: guard against wrong-length config in private helper - is_dominating_set/is_maximal_independent_set: pre-build adjacency list to avoid per-vertex allocation in loop - Remove plan file Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4e809f6 to
cec8a01
Compare
Update MaxCut::unweighted calls to pass SimpleGraph instead of (num_vertices, edges) following the constructor refactor in #74. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…uction paths (#75) * update makefile * feat: implement SAT -> CircuitSAT reduction with tests Converts CNF formulas into boolean circuits by creating an OR gate per clause and a final AND gate constrained to true. Part of Julia parity gaps (issue #67). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add BicliqueCover and BMF Julia parity test fixtures Add export functions for BicliqueCover and BMF in the Julia test data generator, with custom flat-config evaluation logic matching the Rust implementation (since the Julia ProblemReductions.jl package uses incompatible vector-of-vectors / BitMatrix interfaces for these types). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add BicliqueCover and BMF Julia parity tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add reduction-path parity tests matching Julia's test/reduction_path.jl Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add Factoring → SpinGlass chained reduction example Mirrors Julia's examples/Ising.jl — reduces Factoring to SpinGlass via the reduction graph, solves, and extracts factors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: regenerate reduction graph with SAT→CircuitSAT edge Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add reduction path Julia fixtures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: pin random seed in Julia fixture generation Add Random.seed!(42) so re-running generate_testdata.jl produces identical evaluation configs. Reduction path fixtures (deterministic) are skipped if they already exist to avoid slow BruteForce re-solves. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix example * fix: adapt reduction path tests to new graph constructor API Update MaxCut::unweighted calls to pass SimpleGraph instead of (num_vertices, edges) following the constructor refactor in #74. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: revert unrelated Makefile change Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Revert "chore: revert unrelated Makefile change" This reverts commit 1957aeb. * refactor: replace name-level path finding with variant-level API Remove find_paths<S,T>, find_paths_by_name, find_shortest_path<S,T>, and find_shortest_path_by_name. Replace with variant-level equivalents: - find_all_paths(src, src_variant, dst, dst_variant) - find_shortest_path(src, src_variant, dst, dst_variant) All path-finding now operates on exact variant nodes, consistent with find_cheapest_path. Update design.md and all tests accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove redundant cost functions and find_shortest_path Remove find_shortest_path (duplicate of find_cheapest_path with MinimizeSteps). Remove MinimizeWeighted, MinimizeMax, and MinimizeLexicographic cost functions. Keep Minimize, MinimizeSteps, and CustomCost. Update design.md and all tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
new(num_vertices, edges)constructors from all 9 graph problem types that internally construct aSimpleGraphfrom_graph(graph, ...)→new(graph, ...)making the graph topology type explicit in the constructor signaturefrom_graph_unweighted→unweighted,from_graph_unit_weights→unit_weights,from_graph_with_k→with_kWeightElement::IS_UNITconst —is_weighted()is now type-based (returns!W::IS_UNIT) instead of comparing weight values at runtimeis_independent_set,is_vertex_cover,is_clique, etc.) to takegraph: &Ginstead of raw(num_vertices, edges), withassert_eq!on size mismatchCommits
from_graph→newfrom_graph→new,from_graph_with_k→with_kfrom_graph_unweighted→unweighted,from_graph_unit_weights→unit_weightsis_weighted()usingOnemarker typeis_independent_set,is_vertex_cover,is_clique,is_dominating_set,is_maximal_independent_set,is_matching,is_hamiltonian_cycle,is_valid_coloringnow take&impl GraphTest plan
make test clippypassescargo test --docpasses (47 doc tests)new(num_vertices, edges)patternCloses #73
🤖 Generated with Claude Code