feat: add problem_size() to Problem trait with validation#76
feat: add problem_size() to Problem trait with validation#76
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…heapest_path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…omponents Existing tests used arbitrary variable names like "n" and "m" that don't match the overhead polynomial variable names. Updated to use correct names (num_vertices, num_edges) or empty ProblemSize to pass the new validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
- Coverage 96.41% 96.28% -0.13%
==========================================
Files 193 194 +1
Lines 25552 26546 +994
==========================================
+ Hits 24636 25561 +925
- Misses 916 985 +69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…lues Split the `Problem::problem_size()` method into two parts: - `problem_size_names() -> &'static [&'static str]` (type-level, static) - `problem_size_values(&self) -> Vec<usize>` (instance-level) A free function `problem_size()` combines them into a `ProblemSize`. This enables compile-time overhead validation: `ReductionGraph::new()` now asserts that each reduction's overhead input variables are a subset of the source's `problem_size_names`, and output fields are a subset of the target's. This caught and fixed several latent bugs: - `num_elements` → `universe_size` in SetPacking/SetCovering overheads - `num_gates` → `num_variables`/`num_assignments` in Factoring→CircuitSAT - `num_variables` → `num_vars` in SAT→CircuitSAT input - `num_colors` removed from KColoring size (k is a type parameter) - KColoring→ILP/QUBO overheads updated to not reference num_colors Also includes earlier work from this branch: - BicliqueCover refactored to use BipartiteGraph as input - Polynomial composition (mul, pow, substitute) for symbolic overhead - evaluate_path_overhead + compose_path_overhead on ReductionGraph - 3-SAT→MIS triangular overhead test with numeric and symbolic asserts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split ProblemSize into type-level names + instance-level valuesWhat changedThe
A free function WhyThis separation enables compile-time overhead validation: when a reduction rule is registered, the system now checks that:
This caught 6 categories of latent bugs in existing overhead specifications:
Scope
|
| ) -> Option<ReductionPath> { | ||
| let src = self.lookup_node(source, source_variant)?; | ||
|
|
||
| // Validate: when input_size is non-empty, check outgoing edges |
There was a problem hiding this comment.
This validator should be a separate function and get tested.
…ng test - Extract overhead validation logic from ReductionGraph::new() and find_cheapest_path() into standalone validate_overhead_variables() - Add 5 unit tests for the validation function - Replace brute-force SpinGlass solve with ILP solver in test_jl_parity_factoring_to_spinglass_path (>60s → <1s) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolved: extract validator into separate function with testsDone in e2d3ae4 (responding to review comment on the validation logic):
All 1615 tests pass, clippy clean. |
There was a problem hiding this comment.
Pull request overview
Introduces standardized, named problem-size metadata across all Problem implementations, uses it to validate reduction overhead polynomial variable names during path finding, and expands tests/examples accordingly.
Changes:
- Extend
Problemwith size field names + instance values, plus aproblem_size(&P)helper constructingProblemSize. - Add polynomial/reduction-overhead introspection and overhead composition/evaluation utilities; validate overhead variable names in
ReductionGraph. - Update models, reductions, tests, examples, and docs to use consistent size field naming (e.g.,
num_vertices,num_edges,universe_size) and the new validation.
Reviewed changes
Copilot reviewed 100 out of 100 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/suites/reductions.rs | Formatting-only updates in reduction suite tests. |
| tests/suites/integration.rs | Update integration tests (incl. BipartiteGraph usage for BicliqueCover). |
| src/unit_tests/unitdiskmapping_algorithms/common.rs | Formatting-only updates in unit disk mapping test helpers. |
| src/unit_tests/traits.rs | Update test Problems to implement new size-name/value requirements. |
| src/unit_tests/trait_consistency.rs | Update BicliqueCover construction + imports for bipartite graph usage. |
| src/unit_tests/testing/macros.rs | Formatting-only updates in macro tests. |
| src/unit_tests/solvers/brute_force.rs | Update test Problems to implement new size-name/value requirements + formatting. |
| src/unit_tests/rules/travelingsalesman_ilp.rs | Formatting-only updates in TSP→ILP rule tests. |
| src/unit_tests/rules/traits.rs | Update dummy Problems to implement new size-name/value requirements. |
| src/unit_tests/rules/sat_circuitsat.rs | Formatting-only updates in SAT→CircuitSAT rule tests. |
| src/unit_tests/rules/registry.rs | Update reduction registry tests for new size-names fns on entries. |
| src/unit_tests/rules/reduction_path_parity.rs | Add find_cheapest_path test using real problem_size() + ILP-gated factoring test changes. |
| src/unit_tests/rules/minimumvertexcover_qubo.rs | Formatting-only updates in VC→QUBO rule tests. |
| src/unit_tests/rules/minimumvertexcover_minimumsetcovering.rs | Formatting-only updates in VC→SetCovering rule tests. |
| src/unit_tests/rules/minimumvertexcover_maximumindependentset.rs | Formatting-only updates in VC↔IS rule tests. |
| src/unit_tests/rules/minimumvertexcover_ilp.rs | Formatting-only updates in VC→ILP rule tests. |
| src/unit_tests/rules/minimumdominatingset_ilp.rs | Formatting-only updates in DS→ILP rule tests. |
| src/unit_tests/rules/maximummatching_maximumsetpacking.rs | Formatting-only updates in Matching→SetPacking rule tests. |
| src/unit_tests/rules/maximummatching_ilp.rs | Formatting-only updates in Matching→ILP rule tests. |
| src/unit_tests/rules/maximumindependentset_triangular.rs | Formatting-only updates in MIS triangular rule tests. |
| src/unit_tests/rules/maximumindependentset_qubo.rs | Formatting-only updates in MIS→QUBO rule tests. |
| src/unit_tests/rules/maximumindependentset_maximumsetpacking.rs | Formatting-only updates in MIS↔SetPacking rule tests. |
| src/unit_tests/rules/maximumindependentset_ilp.rs | Formatting-only updates in MIS→ILP rule tests. |
| src/unit_tests/rules/maximumindependentset_gridgraph.rs | Formatting-only updates in MIS grid-graph rule tests. |
| src/unit_tests/rules/maximumclique_ilp.rs | Formatting-only updates in Clique→ILP rule tests. |
| src/unit_tests/rules/graph.rs | Update reduction-graph rule tests for new size field names + formatting. |
| src/unit_tests/rules/cost.rs | Remove trailing whitespace line. |
| src/unit_tests/rules/coloring_ilp.rs | Formatting-only updates in Coloring→ILP rule tests. |
| src/unit_tests/reduction_graph.rs | Add overhead composition/evaluation + overhead validation tests; update size names and imports. |
| src/unit_tests/problem_size.rs | New unit tests covering problem_size() for all problem types. |
| src/unit_tests/models/specialized/biclique_cover.rs | Update BicliqueCover tests for BipartiteGraph/local-edge coordinates. |
| src/unit_tests/models/graph/traveling_salesman.rs | Formatting-only updates in TSP model tests. |
| src/unit_tests/models/graph/minimum_vertex_cover.rs | Formatting-only updates in VC model tests. |
| src/unit_tests/models/graph/minimum_dominating_set.rs | Formatting-only updates in DS model tests. |
| src/unit_tests/models/graph/maximum_matching.rs | Formatting-only updates in Matching model tests. |
| src/unit_tests/models/graph/maximum_independent_set.rs | Formatting-only updates in MIS model tests. |
| src/unit_tests/models/graph/maximum_clique.rs | Formatting-only updates in Clique model tests. |
| src/unit_tests/models/graph/maximal_is.rs | Formatting-only updates in MaximalIS model tests. |
| src/unit_tests/models/graph/max_cut.rs | Formatting-only updates in MaxCut model tests. |
| src/unit_tests/models/graph/kcoloring.rs | Formatting-only updates in KColoring model tests. |
| src/unit_tests/io.rs | Formatting-only updates in IO tests. |
| src/unit_tests/graph_models.rs | Formatting-only updates in graph model tests. |
| src/types.rs | Add ProblemSize::from_names_values. |
| src/traits.rs | Add problem size name/value requirements + problem_size(&P) helper. |
| src/rules/sat_minimumdominatingset.rs | Formatting-only update in SAT→DS rule implementation. |
| src/rules/sat_maximumindependentset.rs | Formatting-only update in SAT→MIS rule implementation. |
| src/rules/sat_ksat.rs | Add num_literals to KSat→Sat overhead mapping. |
| src/rules/sat_coloring.rs | Rename SAT→Coloring overhead outputs to match size fields (now includes num_edges). |
| src/rules/sat_circuitsat.rs | Update SAT→CircuitSAT overhead names to match size fields; add assignments output. |
| src/rules/registry.rs | Add overhead input variable introspection, overhead composition helper, and store size-name fns on entries. |
| src/rules/mod.rs | Re-export overhead validation helper for tests. |
| src/rules/minimumvertexcover_minimumsetcovering.rs | Rename overhead output num_elements → universe_size. |
| src/rules/maximumsetpacking_casts.rs | Update cast overhead fields to use universe_size. |
| src/rules/maximummatching_maximumsetpacking.rs | Rename overhead output num_elements → universe_size. |
| src/rules/maximumindependentset_maximumsetpacking.rs | Rename overhead output num_elements → universe_size; minor formatting. |
| src/rules/kcoloring_casts.rs | Update cast overhead fields to remove num_colors and use num_edges. |
| src/rules/graph.rs | Add overhead variable validation (construction-time + query-time), plus overhead composition/evaluation along paths. |
| src/rules/factoring_circuit.rs | Update Factoring→CircuitSAT overhead output field names to match CircuitSAT size fields. |
| src/rules/coloring_qubo.rs | Update KColoring→QUBO overhead for KN variant (k=n) to use num_vertices^2. |
| src/rules/coloring_ilp.rs | Update KColoring→ILP overhead for KN variant (k=n) to use num_vertices^2 and updated constraints expression. |
| src/polynomial.rs | Add variable-name extraction, multiplication/power/substitution, and normalization utilities. |
| src/models/specialized/paintshop.rs | Implement problem size names/values for PaintShop. |
| src/models/specialized/factoring.rs | Implement problem size names/values for Factoring. |
| src/models/specialized/circuit.rs | Implement problem size names/values for CircuitSAT. |
| src/models/specialized/bmf.rs | Implement problem size names/values for BMF. |
| src/models/specialized/biclique_cover.rs | Refactor BicliqueCover to store BipartiteGraph; implement problem size names/values. |
| src/models/set/minimum_set_covering.rs | Implement problem size names/values for MinimumSetCovering. |
| src/models/set/maximum_set_packing.rs | Implement problem size names/values for MaximumSetPacking (compute universe size). |
| src/models/satisfiability/sat.rs | Implement problem size names/values for SAT. |
| src/models/satisfiability/ksat.rs | Implement problem size names/values for K-SAT (compute literals). |
| src/models/optimization/spin_glass.rs | Implement problem size names/values for SpinGlass. |
| src/models/optimization/qubo.rs | Implement problem size names/values for QUBO. |
| src/models/optimization/ilp.rs | Implement problem size names/values for ILP. |
| src/models/graph/traveling_salesman.rs | Implement problem size names/values for TravelingSalesman. |
| src/models/graph/minimum_vertex_cover.rs | Implement problem size names/values for MinimumVertexCover. |
| src/models/graph/minimum_dominating_set.rs | Implement problem size names/values for MinimumDominatingSet. |
| src/models/graph/maximum_matching.rs | Implement problem size names/values for MaximumMatching. |
| src/models/graph/maximum_independent_set.rs | Implement problem size names/values for MaximumIndependentSet. |
| src/models/graph/maximum_clique.rs | Implement problem size names/values for MaximumClique. |
| src/models/graph/maximal_is.rs | Implement problem size names/values for MaximalIS. |
| src/models/graph/max_cut.rs | Implement problem size names/values for MaxCut. |
| src/models/graph/kcoloring.rs | Implement problem size names/values for KColoring. |
| src/lib.rs | Export problem_size from prelude + add problem_size unit test module. |
| problemreductions-macros/src/lib.rs | Extend reduction registry entries with source/target size-name fns. |
| examples/reduction_minimumvertexcover_to_qubo.rs | Formatting-only updates in example. |
| examples/reduction_minimumvertexcover_to_minimumsetcovering.rs | Formatting-only updates in example. |
| examples/reduction_minimumvertexcover_to_maximumindependentset.rs | Formatting-only updates in example. |
| examples/reduction_minimumvertexcover_to_ilp.rs | Formatting-only updates in example. |
| examples/reduction_minimumdominatingset_to_ilp.rs | Formatting-only updates in example. |
| examples/reduction_maximummatching_to_maximumsetpacking.rs | Formatting-only updates in example. |
| examples/reduction_maximummatching_to_ilp.rs | Formatting-only updates in example. |
| examples/reduction_maximumindependentset_to_qubo.rs | Formatting-only updates in example. |
| examples/reduction_maximumindependentset_to_minimumvertexcover.rs | Formatting-only updates in example. |
| examples/reduction_maximumindependentset_to_maximumsetpacking.rs | Formatting-only updates in example. |
| examples/reduction_maximumindependentset_to_ilp.rs | Formatting-only updates in example. |
| examples/reduction_maximumclique_to_ilp.rs | Formatting-only updates in example. |
| examples/chained_reduction_factoring_to_spinglass.rs | Formatting-only updates in example. |
| docs/plans/2026-02-16-problem-size-trait-impl.md | Add implementation plan document for problem size + overhead validation. |
| docs/paper/reductions.typ | Update paper text to reference problem-reductions naming. |
| benches/solver_benchmarks.rs | Formatting-only updates in benchmarks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn from_names_values(names: &[&str], values: &[usize]) -> Self { | ||
| debug_assert_eq!( | ||
| names.len(), | ||
| values.len(), | ||
| "ProblemSize: names ({}) and values ({}) length mismatch", | ||
| names.len(), | ||
| values.len() | ||
| ); | ||
| Self { | ||
| components: names | ||
| .iter() | ||
| .zip(values.iter()) | ||
| .map(|(k, v)| (k.to_string(), *v)) | ||
| .collect(), | ||
| } |
There was a problem hiding this comment.
ProblemSize::from_names_values only uses debug_assert_eq! to check names.len() == values.len(). In release builds a mismatch will silently truncate due to zip, producing an incorrect ProblemSize and potentially masking bad problem_size_values() implementations. Consider using assert_eq! (or returning a Result) so mismatches are always caught.
| #[reduction( | ||
| overhead = { | ||
| ReductionOverhead::new(vec![ | ||
| // 2*num_vars + 3 (base) + 5*(num_literals - num_clauses) (OR gadgets) | ||
| ("num_vertices", poly!(2 * num_vars) + poly!(5 * num_literals) + poly!(num_clauses).scale(-5.0) + poly!(3)), | ||
| ("num_colors", poly!(3)), | ||
| ("num_edges", poly!(2 * num_vars) + poly!(5 * num_literals) + poly!(num_clauses).scale(-5.0) + poly!(3)), | ||
| ]) |
There was a problem hiding this comment.
The SAT→KColoring overhead now reports a num_edges field but uses the same polynomial as num_vertices. Based on the constructor, edges grow as 3 + 3*num_vars + 11*(num_literals - num_clauses) + 2*num_clauses (triangle + per-var edges + OR-gadget edges + per-clause set_true edges), so equating edges to vertices will misestimate costs and path selection. Update the num_edges overhead polynomial to reflect how many edges are actually added.
| fn variant() -> Vec<(&'static str, &'static str)>; | ||
| /// Type-level: fixed field names for this problem type's size metrics. | ||
| /// | ||
| /// Every instance of this problem type uses the same set of field names, | ||
| /// so this is a static method. | ||
| fn problem_size_names() -> &'static [&'static str]; | ||
| /// Instance-level: values for each size field (same order as `problem_size_names()`). | ||
| fn problem_size_values(&self) -> Vec<usize>; | ||
| } | ||
|
|
||
| /// Combine type-level names and instance-level values into a [`ProblemSize`]. | ||
| pub fn problem_size<P: Problem>(p: &P) -> crate::types::ProblemSize { | ||
| crate::types::ProblemSize::from_names_values(P::problem_size_names(), &p.problem_size_values()) | ||
| } |
There was a problem hiding this comment.
PR description/title mention adding a required problem_size() method on Problem returning ProblemSize, but the implementation instead adds problem_size_names() + problem_size_values() and a free function problem_size(&P). If the intent is to expose a problem_size() method on the trait/public API, consider aligning the code/API and docs (either add a default fn problem_size(&self) -> ProblemSize on the trait using the two helpers, or update the PR description accordingly).
- Use assert_eq! instead of debug_assert_eq! in from_names_values() so length mismatches are caught in release builds - Fix SAT→KColoring num_edges polynomial: 3*num_vars + 11*num_literals - 9*num_clauses + 3 (was incorrectly copied from num_vertices) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolved Copilot review comments (ac3faa5)
|
ee17ddb to
ff13af4
Compare
Show evaluate_path_overhead and compose_path_overhead in the chained reduction example. The getting-started guide now explains how to inspect problem size growth along a reduction path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ff13af4 to
4b7b5a2
Compare
9 tasks covering: internalize reduction structs, unitdiskmapping internals, polynomial/truth_table/graph_types modules, add is_valid_solution methods, internalize validation free functions, config consolidation, prelude update. Updated for PR #76 (problem_size trait additions). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
problem_size()method toProblemtrait returningProblemSizewith named size componentsproblem_size()for all 21 problem types across graph, SAT, set, optimization, and specialized categoriesvariable_names()toPolynomialandinput_variable_names()toReductionOverheadfor overhead introspectionproblem_size()components infind_cheapest_path(catches naming mismatches at path-finding time)find_cheapest_pathwith real problem sizesTest plan
test_problem_size_*unit tests pass verifying correct component names and valuestest_find_cheapest_path_with_problem_sizeintegration test passes with real Petersen graphmake clippypasses with zero warningsmake fmtapplied🤖 Generated with Claude Code