Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #602 +/- ##
==========================================
+ Coverage 96.76% 96.77% +0.01%
==========================================
Files 226 226
Lines 29949 29919 -30
==========================================
- Hits 28981 28955 -26
+ Misses 968 964 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add SubsetSum satisfaction problem to src/models/misc/ - Binary variables: x_i = 1 if element i is in the subset - Evaluates true iff selected subset sums to target B - Complexity: 2^(n/2) via Horowitz-Sahni meet-in-the-middle - CLI: pred create SubsetSum --sizes 3,7,1,8,2,4 --target 11 - 15 unit tests covering basic, edge cases, brute force, serialization - Regenerated problem schemas JSON Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implementation SummaryChanges
Deviations from Plan
Open Questions
|
There was a problem hiding this comment.
Pull request overview
Adds a new SubsetSum satisfaction problem model to the library, along with CLI wiring and documentation schema updates so instances can be created/loaded/serialized consistently across the ecosystem.
Changes:
- Introduces
SubsetSummodel (Problem<Metric=bool> + SatisfactionProblem) with schema registration and variants declaration. - Adds unit tests covering evaluation behavior, brute-force solving, and serde roundtrips.
- Registers
SubsetSumin the CLI (name resolution, create command, dispatch load/serialize) and updates docs problem schema JSON.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/models/misc/subset_sum.rs | New SubsetSum problem model + registry schema + variants + tests module hook |
| src/unit_tests/models/misc/subset_sum.rs | Unit tests for SubsetSum evaluation/solver/serialization |
| src/models/misc/mod.rs | Exposes SubsetSum from models::misc |
| src/lib.rs | Adds SubsetSum to the crate prelude |
| problemreductions-cli/src/problem_name.rs | Adds subsetsum alias resolution |
| problemreductions-cli/src/dispatch.rs | Adds load/serialize dispatch for SubsetSum |
| problemreductions-cli/src/commands/create.rs | Adds pred create SubsetSum --sizes ... --target ... support |
| problemreductions-cli/src/cli.rs | Documents SubsetSum flags in CLI help text |
| docs/src/reductions/problem_schemas.json | Adds SubsetSum entry to exported problem schema list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/models/misc/subset_sum.rs
Outdated
| /// Given a finite set A with sizes `s(a) ∈ Z⁺` for each element and a | ||
| /// positive integer B, determine whether there exists a subset `A' ⊆ A` | ||
| /// such that `∑_{a ∈ A'} s(a) = B`. |
There was a problem hiding this comment.
The docs/spec say the target B is a “positive integer”, but the implementation (u64) and unit tests explicitly allow target = 0 (e.g., empty set / target 0). Please either (a) update the documentation to describe B as nonnegative, or (b) enforce target > 0 in new() (and adjust/remove the target=0 tests accordingly) so the docs match behavior.
| { | ||
| "name": "SubsetSum", | ||
| "description": "Decide if a subset of positive integers sums to a target value", | ||
| "fields": [ | ||
| { | ||
| "name": "sizes", | ||
| "type_name": "Vec<u64>", | ||
| "description": "Positive integer size s(a) for each element a in A" | ||
| }, | ||
| { | ||
| "name": "target", | ||
| "type_name": "u64", | ||
| "description": "Target sum B" | ||
| } | ||
| ] |
There was a problem hiding this comment.
problem_schemas.json was updated for SubsetSum, but docs/src/reductions/reduction_graph.json is also generated from the registry and currently does not include any SubsetSum node/entry. Please regenerate and commit the updated reduction_graph.json (e.g., cargo run --example export_graph) so the docs’ JSON resources stay consistent.
Resolve conflicts: prefer main's i64/num_elements convention for SubsetSum, keep additional test coverage, combine CLI additions (GraphPartitioning, MinimumFeedbackVertexSet, SubsetSum). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address Copilot review comment: ensure SubsetSum appears in both generated JSON docs files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix type_format_hint() to handle Vec<i64> and i64 types, and add SubsetSum entry to example_for() so `pred create SubsetSum` shows a helpful usage example. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Pipeline Report
🤖 Generated by review-pipeline |
- Fix corrupted reduction_graph.json that had stdout text mixed into the file - Apply rustfmt formatting to two example files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # problemreductions-cli/src/commands/create.rs
Summary
src/models/misc/Fixes #402