Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
=======================================
Coverage 96.36% 96.36%
=======================================
Files 197 197
Lines 27210 27210
=======================================
Hits 26220 26220
Misses 990 990 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive implementation plan for the ClosestVectorProblem (CVP) optimization model. CVP is a fundamental lattice problem where, given a lattice basis B and target vector t, the goal is to find integer coefficients x that minimize the Euclidean distance ‖Bx - t‖₂.
The plan follows the established add-model skill pattern with 7 tasks covering all aspects of model integration: struct definition, trait implementations, module/CLI registration, comprehensive testing, and paper documentation. The implementation parameterizes CVP by element type T (supporting both i32 for integer lattices and f64 for real lattices) and reuses VarBounds from ILP for specifying enumeration bounds on integer variables.
Changes:
- Adds detailed implementation plan with step-by-step TDD approach for CVP model
- Includes struct design with basis matrix (column vectors), target vector, and variable bounds
- Provides comprehensive test suite covering evaluation, solvers, serialization, and edge cases
- Adds paper documentation with mathematical definition matching issue #90 specification
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Add match arm in `serialize_any_problem()` (after the `"ILP"` arm): | ||
| ```rust | ||
| "ClosestVectorProblem" => try_ser::<ClosestVectorProblem<i32>>(data), |
There was a problem hiding this comment.
Similarly, the serialize_any_problem function should support both i32 and f64 variants. Following the SpinGlass pattern (lines 281-284), this should check the variant map:
"ClosestVectorProblem" => match variant.get("weight").map(|s| s.as_str()) {
Some("f64") => try_ser::<ClosestVectorProblem<f64>>(any),
_ => try_ser::<ClosestVectorProblem<i32>>(any),
},
| Add match arm in `serialize_any_problem()` (after the `"ILP"` arm): | |
| ```rust | |
| "ClosestVectorProblem" => try_ser::<ClosestVectorProblem<i32>>(data), | |
| Add match arm in `serialize_any_problem()` (after the `"ILP"` arm), following the SpinGlass pattern and using the `variant` map to select the weight type: | |
| ```rust | |
| "ClosestVectorProblem" => match variant.get("weight").map(|s| s.as_str()) { | |
| Some("f64") => try_ser::<ClosestVectorProblem<f64>>(data), | |
| _ => try_ser::<ClosestVectorProblem<i32>>(data), | |
| }, |
| Add to `src/models/optimization/closest_vector_problem.rs`, requiring `T: Into<f64> + Clone`: | ||
|
|
||
| ```rust | ||
| impl<T: Clone + Into<f64> + Serialize + for<'de> Deserialize<'de> + std::fmt::Debug + 'static> Problem for ClosestVectorProblem<T> { |
There was a problem hiding this comment.
The trait bounds should include crate::variant::VariantParam for type T. This is required for the variant system to work properly with the variant_params macro. Following the pattern from SpinGlass (line 200-202 in src/models/optimization/spin_glass.rs), the bounds should be:
T: Clone + Into<f64> + crate::variant::VariantParam + Serialize + for<'de> Deserialize<'de> + std::fmt::Debug + 'static
| impl<T: Clone + Into<f64> + Serialize + for<'de> Deserialize<'de> + std::fmt::Debug + 'static> Problem for ClosestVectorProblem<T> { | |
| impl<T: Clone + Into<f64> + crate::variant::VariantParam + Serialize + for<'de> Deserialize<'de> + std::fmt::Debug + 'static> Problem for ClosestVectorProblem<T> { |
| } | ||
| } | ||
|
|
||
| impl<T: Clone + Into<f64> + Serialize + for<'de> Deserialize<'de> + std::fmt::Debug + 'static> OptimizationProblem for ClosestVectorProblem<T> { |
There was a problem hiding this comment.
The OptimizationProblem trait implementation should also include crate::variant::VariantParam in the trait bounds for type T, matching the Problem trait implementation and following the pattern from SpinGlass (line 237-238).
| impl<T: Clone + Into<f64> + Serialize + for<'de> Deserialize<'de> + std::fmt::Debug + 'static> OptimizationProblem for ClosestVectorProblem<T> { | |
| impl<T: Clone + Into<f64> + Serialize + for<'de> Deserialize<'de> + std::fmt::Debug + crate::variant::VariantParam + 'static> OptimizationProblem for ClosestVectorProblem<T> { |
| } | ||
|
|
||
| fn variant() -> Vec<(&'static str, &'static str)> { | ||
| crate::variant_params![] |
There was a problem hiding this comment.
The variant function should declare the type parameter T since ClosestVectorProblem is parameterized by element type. Both i32 and f64 implement VariantParam with category "weight" (see src/types.rs:248-249), so this should be crate::variant_params![T] instead of crate::variant_params![]. Without this, the variant system won't properly track the i32 vs f64 distinction, which could cause issues with serialization and CLI dispatch.
| crate::variant_params![] | |
| crate::variant_params![T] |
|
|
||
| ```rust | ||
| use super::*; | ||
| use crate::models::optimization::VarBounds; |
There was a problem hiding this comment.
This import is redundant. The test file already uses super::* at line 23, which imports all public items from the parent module (optimization), including VarBounds. This explicit import can be removed.
| use crate::models::optimization::VarBounds; |
| "ClosestVectorProblem" => deser_opt::<ClosestVectorProblem<i32>>(data), | ||
| ``` |
There was a problem hiding this comment.
The CLI dispatch should support both i32 and f64 variants of ClosestVectorProblem, as specified in issue #90. Currently only i32 is registered. Following the pattern from SpinGlass (lines 230-233 in problemreductions-cli/src/dispatch.rs), this should check the variant map and dispatch to the appropriate type. The match arm should be:
"ClosestVectorProblem" => match variant.get("weight").map(|s| s.as_str()) {
Some("f64") => deser_opt::<ClosestVectorProblem<f64>>(data),
_ => deser_opt::<ClosestVectorProblem<i32>>(data),
},
Summary
Plan Tasks
Closes #90