Skip to content

Fix #532: [Model] IntegerKnapsack#815

Merged
zazabap merged 5 commits intomainfrom
issue-532-integer-knapsack
Mar 29, 2026
Merged

Fix #532: [Model] IntegerKnapsack#815
zazabap merged 5 commits intomainfrom
issue-532-integer-knapsack

Conversation

@isPANN
Copy link
Copy Markdown
Collaborator

@isPANN isPANN commented Mar 28, 2026

Summary

  • Add IntegerKnapsack model to src/models/set/ — the unbounded knapsack variant where items can be selected with non-negative integer multiplicities
  • Value type Max<i64>, variable domains {0,...,floor(B/s_i)} per item, complexity (capacity+1)^num_items
  • CLI create support (pred create IntegerKnapsack --sizes ... --values ... --capacity ...), canonical example-db entry, paper entry with example

close #532

Test plan

  • 20 unit tests covering creation, evaluation (feasible, infeasible, empty, out-of-domain), brute force solving, serialization, deserialization validation, edge cases, and paper example verification
  • make fmt-check passes
  • make clippy passes
  • make test passes (all workspace tests)
  • make paper compiles
  • Example-db tests pass

🤖 Generated with Claude Code

…iplicities)

Closes #532. Adds IntegerKnapsack to `src/models/set/` with Max<i64>
value type, per-item variable domains {0,...,floor(B/s_i)}, CLI create
support, canonical example, paper entry, and 20 unit tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.86%. Comparing base (6a4f5c7) to head (f8d785b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #815    +/-   ##
========================================
  Coverage   97.85%   97.86%            
========================================
  Files         633      635     +2     
  Lines       69346    69612   +266     
========================================
+ Hits        67859    68126   +267     
+ Misses       1487     1486     -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@GiggleLiu
Copy link
Copy Markdown
Contributor

Agentic Review Report

Structural Check

Structural Review: model IntegerKnapsack

Structural Completeness

# Check Status
1 Model file exists PASS — src/models/set/integer_knapsack.rs:1
2 inventory::submit! present PASS — src/models/set/integer_knapsack.rs:11
3 Serialize / Deserialize derive on struct PASS — src/models/set/integer_knapsack.rs:50
4 Problem trait impl PASS — src/models/set/integer_knapsack.rs:112
5 Aggregate value present PASS — type Value = Max<i64> at src/models/set/integer_knapsack.rs:114
6 #[cfg(test)] + #[path = ...] test link PASS — src/models/set/integer_knapsack.rs:208
7 Test file exists PASS — src/unit_tests/models/set/integer_knapsack.rs:1
8 Test file has >= 3 test functions PASS — 19 test_ functions in src/unit_tests/models/set/integer_knapsack.rs:6
9 Registered in set/mod.rs PASS — src/models/set/mod.rs:17, src/models/set/mod.rs:30
10 Re-exported in models/mod.rs PASS — src/models/mod.rs:51
11 Variant registration exists PASS — src/models/set/integer_knapsack.rs:152
12 CLI resolve_alias entry PASS — alias resolution is registry-driven in problemreductions-cli/src/problem_name.rs:16, backed by the schema entry in src/models/set/integer_knapsack.rs:11
13 CLI create support PASS — example hint at problemreductions-cli/src/commands/create.rs:719 and handler at problemreductions-cli/src/commands/create.rs:2323
14 Canonical model example registered PASS — example spec in src/models/set/integer_knapsack.rs:192, wired through src/models/set/mod.rs:40 and src/example_db/model_builders.rs:3
15 Paper display-name entry PASS — docs/paper/reductions.typ:114
16 Paper problem-def block PASS — docs/paper/reductions.typ:4035
17 No blacklisted autogenerated files committed PASS — diff touches none of the blacklisted generated paths

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • Serialization/validation expectations: ISSUE — Deserialize only validates fields individually at src/models/set/integer_knapsack.rs:50. A JSON payload with mismatched sizes / values lengths bypasses the constructor invariant in src/models/set/integer_knapsack.rs:66, and evaluate() later indexes self.values[i] by self.sizes.len() at src/models/set/integer_knapsack.rs:135, so malformed input can panic instead of being rejected.
  • evaluate() correctness: OK — for well-formed instances it rejects wrong-length, out-of-domain, and overweight configs before returning the objective value at src/models/set/integer_knapsack.rs:127.
  • dims() correctness: OK — one multiplicity variable per item with domain size floor(capacity / size_i) + 1 at src/models/set/integer_knapsack.rs:120.
  • Size getter consistency: OK — capacity() and num_items() exist and match the complexity metadata used by src/models/set/integer_knapsack.rs:152; see src/models/set/integer_knapsack.rs:101 and src/models/set/integer_knapsack.rs:106.
  • Weight handling: OK — not a graph/weight-parameterized model.
  • Paper/model definition match: OK — schema fields in src/models/set/integer_knapsack.rs:19 and the paper definition in docs/paper/reductions.typ:4035 agree with the implemented model.

Issue Compliance

# Check Status
1 Problem name matches issue OK — IntegerKnapsack
2 Mathematical definition matches OK — maximize Σ c_i v_i subject to Σ c_i s_i ≤ B in src/models/set/integer_knapsack.rs:27 and docs/paper/reductions.typ:4035
3 Problem framing matches OK — optimization model with Value = Max<i64> at src/models/set/integer_knapsack.rs:114
4 Type parameters match OK — no extra type parameters or threshold/bound field added
5 Configuration space matches OK — dims() encodes {0, ..., floor(B/s_i)} per item at src/models/set/integer_knapsack.rs:120
6 Feasibility check matches OK — infeasible / out-of-domain configs return Max(None) at src/models/set/integer_knapsack.rs:127
7 Objective function matches OK — evaluate() computes Σ c_i * values[i] at src/models/set/integer_knapsack.rs:143
8 Complexity matches OK — declare_variants! uses (capacity + 1)^num_items at src/models/set/integer_knapsack.rs:152

Summary

  • 17/17 structural checks passed
  • 8/8 issue compliance checks passed
  • ISSUE — malformed JSON with mismatched sizes / values lengths can deserialize successfully and later panic in evaluate() instead of failing validation cleanly; see src/models/set/integer_knapsack.rs:50, src/models/set/integer_knapsack.rs:66, and src/models/set/integer_knapsack.rs:135.

Quality Check

Quality Review

Design Principles

  • DRY: ISSUE — validation is split between field-level serde hooks and IntegerKnapsack::new, but the two paths enforce different invariants. Deserialization checks positivity only, while constructor-only length checks mean malformed JSON can create an invalid model state that later panics during evaluation. src/models/set/integer_knapsack.rs:52, src/models/set/integer_knapsack.rs:66, src/models/set/integer_knapsack.rs:143
  • KISS: ISSUE — the CLI create path reuses an assertion-based constructor for user input, so ordinary validation failures become panics instead of structured errors. A fallible validation path would be simpler and safer at the API boundary. problemreductions-cli/src/commands/create.rs:2323, problemreductions-cli/src/commands/create.rs:2341, src/models/set/integer_knapsack.rs:67
  • HC/LC: OK

HCI (if CLI/MCP changed)

  • Error messages: ISSUE — pred create IntegerKnapsack will panic on mismatched lengths, non-positive sizes/values, or negative capacity because the branch forwards parsed user input straight into IntegerKnapsack::new. That bypasses actionable CLI diagnostics for exactly the invalid inputs users are most likely to make. problemreductions-cli/src/commands/create.rs:2323, problemreductions-cli/src/commands/create.rs:2341, src/models/set/integer_knapsack.rs:67
  • Discoverability: OK
  • Consistency: OK
  • Least surprise: ISSUE — the new model registers brute-force complexity as (capacity + 1)^num_items, while the paper entry in the same PR states the standard exact dynamic program is O(n B). Registry-backed CLI surfaces will therefore report a worse complexity than the project’s own documentation claims is known. src/models/set/integer_knapsack.rs:152, docs/paper/reductions.typ:4038
  • Feedback: OK

Test Quality

  • Naive test detection: ISSUE
    • test_integer_knapsack_deserialization_rejects_invalid_fields covers sign checks but misses mismatched sizes/values lengths, which is the serialized-state bug that currently bypasses constructor validation and can later panic in evaluate/solver code. src/unit_tests/models/set/integer_knapsack.rs:156, src/models/set/integer_knapsack.rs:52, src/models/set/integer_knapsack.rs:143

Issues

Critical (Must Fix)

  • Deserializing IntegerKnapsack does not enforce sizes.len() == values.len(), so malformed JSON can construct an invalid instance that later panics when evaluate indexes self.values[i]. This is a real crash path for loaded instances and for any solver that evaluates them. src/models/set/integer_knapsack.rs:52, src/models/set/integer_knapsack.rs:66, src/models/set/integer_knapsack.rs:143

Important (Should Fix)

  • The new pred create IntegerKnapsack branch can panic on invalid user input because it calls IntegerKnapsack::new directly. Bad CLI input should return Result errors, not abort the process via assert!. problemreductions-cli/src/commands/create.rs:2323, problemreductions-cli/src/commands/create.rs:2341, src/models/set/integer_knapsack.rs:67
  • The registered best-known complexity is inconsistent with the same PR’s paper text. declare_variants! advertises brute-force (capacity + 1)^num_items, but the paper documents the standard exact O(n B) DP; registry-backed complexity output will therefore be misleading. src/models/set/integer_knapsack.rs:152, docs/paper/reductions.typ:4038

Minor (Nice to Have)

  • None.

Summary

  • Critical: deserialization can build an invalid IntegerKnapsack with mismatched vector lengths, leading to panics later. src/models/set/integer_knapsack.rs:52, src/models/set/integer_knapsack.rs:143
  • Important: pred create IntegerKnapsack crashes on invalid input because it uses an assertion-based constructor as a CLI validation path. problemreductions-cli/src/commands/create.rs:2323, problemreductions-cli/src/commands/create.rs:2341
  • Important: the model’s registered complexity metadata contradicts the paper entry and will mislead registry/CLI consumers. src/models/set/integer_knapsack.rs:152, docs/paper/reductions.typ:4038

Agentic Feature Tests

Agentic Feature Tests

Summary

I tested the pred list / pred show / pred create --example / pred solve flow in the current worktree. The CLI exposes Knapsack as the canonical problem name; IntegerKnapsack is not present in the catalog or example DB. The documented Knapsack path works end to end.

Test Results

  • cargo run -q -p problemreductions-cli --bin pred -- list | rg -n "Knapsack|IntegerKnapsack"
    • Output included Knapsack * and PartiallyOrderedKnapsack *; there was no IntegerKnapsack entry.
  • cargo run -q -p problemreductions-cli --bin pred -- show Knapsack
    • Succeeded and displayed the problem summary, fields, complexity, and reductions.
  • cargo run -q -p problemreductions-cli --bin pred -- show IntegerKnapsack
    • Failed with Error: Unknown problem: IntegerKnapsack.
  • cargo run -q -p problemreductions-cli --bin pred -- create --example Knapsack -o /tmp/knapsack.json
    • Succeeded with Wrote /tmp/knapsack.json.
  • cargo run -q -p problemreductions-cli --bin pred -- solve /tmp/knapsack.json
    • Succeeded and returned a valid solution for Knapsack.
  • cargo run -q -p problemreductions-cli --bin pred -- create --example IntegerKnapsack -o /tmp/integer-knapsack.json
    • Failed with Error: Unknown problem: IntegerKnapsack.

Issues Found

  • High, confirmed: IntegerKnapsack is not a valid CLI/catalog/example name in this worktree. A downstream user following that name cannot discover the feature, show its details, or create the example instance. The working canonical name is Knapsack.

Reproduction / Classification

  • pred list does not show IntegerKnapsack.
  • pred show IntegerKnapsack fails with Unknown problem: IntegerKnapsack.
  • pred create --example IntegerKnapsack fails with Unknown problem: IntegerKnapsack.
  • Classification: confirmed.

Doc Suggestions

  • Align user-facing docs and examples with the canonical name Knapsack, or add IntegerKnapsack as an explicit alias if that name is intended to be supported.
  • If the PR or issue text uses IntegerKnapsack, add a note in the CLI docs that Knapsack is the canonical catalog name so users do not hit the unknown-problem error.

Verdict

fail


Generated by review-pipeline

@isPANN isPANN mentioned this pull request Mar 29, 2026
3 tasks
…apsack

Use TryFrom<RawIntegerKnapsack> pattern to validate cross-field invariants
(sizes/values length match) during deserialization, preventing panics from
malformed JSON. Add anyhow::ensure! checks in CLI create path for friendly
error messages instead of assertion panics on invalid user input.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@zazabap zazabap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zazabap zazabap merged commit 102b941 into main Mar 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Model] IntegerKnapsack

3 participants