feat: Implement Factoring → ILP reduction (issue #21)#22
Conversation
Design for issue #21 - integer programming based Factoring solver. Uses McCormick linearization for binary products with carry propagation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Shows closed-loop test pattern: create problem, reduce to ILP, solve, extract solution, verify correctness. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Documents the closed-loop test pattern matching the typst example. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Register reduction in mod.rs - Add edge in graph.rs type registry - Update reduction graph documentation - Regenerate reduction_graph.json 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
==========================================
+ Coverage 98.04% 98.07% +0.02%
==========================================
Files 59 60 +1
Lines 12265 12550 +285
==========================================
+ Hits 12025 12308 +283
- Misses 240 242 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Implements a new reduction from the specialized Factoring problem to ILP using McCormick linearization plus carry propagation, and updates the reduction graph documentation accordingly.
Changes:
- Add
Factoring -> ILPreduction rule with inventory registration and solution extraction. - Extend the runtime reduction graph (type registry + documented edges) to include ILP as a target for Factoring.
- Update docs (reduction graph diagrams + paper theorem/example + regenerated JSON graph data).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rules/mod.rs | Registers and re-exports the new factoring_ilp reduction behind the ilp feature. |
| src/rules/graph.rs | Adds ILP as a registered type and includes a Factoring→ILP edge in the graph. |
| src/rules/factoring_ilp.rs | New Factoring→ILP reduction implementation (McCormick + carries) with comprehensive tests. |
| docs/src/reductions/graph.md | Updates the reduction graph diagram to show ILP and the new Factoring→ILP edge. |
| docs/plans/2026-01-31-factoring-ilp-design.md | Adds a design document for the reduction formulation and test plan. |
| docs/paper/reductions.typ | Adds a theorem/proof sketch and a closed-loop Rust example for Factoring→ILP. |
| docs/paper/reduction_graph.json | Regenerates the paper reduction graph data to include Factoring→ILP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| - McCormick, G. P. (1976). Computability of global solutions to factorable nonconvex programs. Mathematical Programming. | ||
| - Issue #3: Coding rules for AI agents | ||
| - Existing implementation: `src/rules/coloring_ilp.rs` (pattern reference) |
There was a problem hiding this comment.
The design doc references src/rules/coloring_ilp.rs as an existing pattern, but that file doesn’t exist in this repo (no matches under src/rules). Please update the reference to a real file (e.g., src/rules/vertexcovering_ilp.rs, src/rules/clique_ilp.rs) or remove it to avoid confusing readers.
| - Existing implementation: `src/rules/coloring_ilp.rs` (pattern reference) | |
| - Existing implementation: `src/rules/vertexcovering_ilp.rs` (pattern reference) |
src/rules/graph.rs
Outdated
| add_edge!("CircuitSAT" => "SpinGlass"); | ||
| add_edge!("Factoring" => "CircuitSAT"); | ||
|
|
||
| // ILP reductions (for solver targets) |
There was a problem hiding this comment.
add_edge!("Factoring" => "ILP") is currently added unconditionally, which makes the reduction graph advertise a Factoring→ILP edge even when the ilp feature is disabled (the actual ReduceTo<ILP> for Factoring impl lives behind that feature). Please gate this edge behind #[cfg(feature = "ilp")] or remove the manual edge and rely solely on the inventory-registered reduction when the feature is enabled.
| // ILP reductions (for solver targets) | |
| // ILP reductions (for solver targets) | |
| #[cfg(feature = "ilp")] |
src/rules/factoring_ilp.rs
Outdated
| // RHS is N_k (k-th bit of target) | ||
| let n_k = ((target >> k) & 1) as f64; |
There was a problem hiding this comment.
((target >> k) & 1) will panic when k >= 64 (shift overflow on u64). Since k iterates up to num_bit_positions and m + n can exceed 64, this can crash the reduction for larger bit-width settings. Compute N_k with a guard (e.g., treat bits beyond 63 as 0) or cap the modeled bit positions to the representable width of target.
| // RHS is N_k (k-th bit of target) | |
| let n_k = ((target >> k) & 1) as f64; | |
| // RHS is N_k (k-th bit of target). For k >= 64, the bit is 0 for u64. | |
| let n_k = if k < 64 { | |
| ((target >> k) & 1) as f64 | |
| } else { | |
| 0.0 | |
| }; |
| // Number of bit positions to check: max(m+n, target_bits) | ||
| // This ensures we catch infeasible cases where target is too large | ||
| let num_bit_positions = std::cmp::max(m + n, target_bits); | ||
|
|
||
| // Total variables: m + n + m*n + num_bit_positions | ||
| let num_p = m; | ||
| let num_q = n; | ||
| let num_z = m * n; | ||
| let num_carries = num_bit_positions; | ||
| let num_vars = num_p + num_q + num_z + num_carries; |
There was a problem hiding this comment.
The reduction sizes are documented/registered as depending on m and n only (inventory overhead: carries = m+n, constraints = 3mn + (m+n) + 1), but the implementation uses num_bit_positions = max(m+n, target_bits) which makes num_vars/constraints depend on target for large targets. This breaks the overhead formulas and diverges from the stated formulation (c_k for k=0..m+n-1, c_{m+n-1}=0). Consider keeping num_bit_positions = m+n and handling target_bits > m+n by emitting an immediately-infeasible ILP (or a single unsatisfiable constraint) instead of adding extra carry positions.
| ```rust | ||
| use problemreductions::prelude::*; | ||
|
|
||
| // 1. Create factoring instance: find p (4-bit) × q (4-bit) = 15 | ||
| let problem = Factoring::new(4, 4, 15); | ||
|
|
||
| // 2. Reduce to ILP | ||
| let reduction = ReduceTo::<ILP>::reduce_to(&problem); | ||
| let ilp = reduction.target_problem(); | ||
|
|
||
| // 3. Solve ILP | ||
| let solver = ILPSolver::new(); | ||
| let ilp_solution = solver.solve(ilp).unwrap(); |
There was a problem hiding this comment.
The example uses ILP / ILPSolver, which are behind the crate’s ilp feature. Consider noting in the paper snippet that consumers must enable the ilp feature (e.g., in Cargo.toml or via --features ilp), otherwise this example won’t compile when copied.
docs/paper/reduction_graph.json
Outdated
| { | ||
| "nodes": [ | ||
| { | ||
| "id": "MaxCut", | ||
| "label": "MaxCut", | ||
| "id": "IndependentSet", | ||
| "label": "IndependentSet", | ||
| "category": "graph" | ||
| }, | ||
| { |
There was a problem hiding this comment.
This regenerated JSON includes large ordering-only changes (nodes/edges reordered) beyond the new Factoring→ILP edge, which will create noisy diffs going forward. Consider making the generator output deterministic (e.g., sort nodes by id and edges by (source,target) before serializing) so future graph regenerations are stable.
- Fix design doc reference to use existing vertexcovering_ilp.rs - Add #[cfg(feature = "ilp")] gate to Factoring→ILP edge in graph.rs - Add shift overflow guard for k >= 64 in bit extraction - Add feature requirement note to typst example - Clarify overhead formula comments for infeasible cases 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sort nodes by id and edges by (source, target) to ensure stable output across runs. Regenerate reduction_graph.json with sorted data. Addresses PR review comment #6. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
inventory::submit! already adds edges automatically. Manual add_edge! is only needed for legacy reductions without inventory registration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
All reductions now use inventory::submit! for auto-discovery. The register_reductions function is kept as a template for future manual registrations if needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
All reductions use inventory::submit! for auto-discovery. No need for manual registration function. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Factoring → ILPreduction using McCormick linearization with carry propagationImplementation Details
Variables:
p_i, q_j ∈ {0,1}for factor bitsz_ij ∈ {0,1}for productsp_i × q_jc_k ≥ 0for carries at each bit positionConstraints:
z_ij ≤ p_i,z_ij ≤ q_j,z_ij ≥ p_i + q_j - 1Σ_{i+j=k} z_ij + c_{k-1} = N_k + 2·c_kc_{m+n-1} = 0Complexity: O(m×n) variables and constraints for m-bit × n-bit factorization.
Test Plan
Closes #21
🤖 Generated with Claude Code