Skip to content

feat: add IntegerExpressionMembership model (#552)#814

Merged
isPANN merged 5 commits intomainfrom
552-integer-expression-membership
Mar 29, 2026
Merged

feat: add IntegerExpressionMembership model (#552)#814
isPANN merged 5 commits intomainfrom
552-integer-expression-membership

Conversation

@isPANN
Copy link
Copy Markdown
Collaborator

@isPANN isPANN commented Mar 28, 2026

Summary

  • Add IntegerExpressionMembership problem model: given a recursive integer expression tree over union and Minkowski sum on singleton positive integers, decide if target K is in the represented set
  • IntExpr enum (Atom/Union/Sum) with Serialize/Deserialize support
  • Value = Or (satisfaction problem), dims() = vec![2; num_union_nodes]
  • Getters: expression_size(), num_union_nodes(), num_atoms(), expression_depth()
  • Complexity: 2^num_union_nodes (brute force)
  • CLI: pred create IntegerExpressionMembership --expression <JSON> --target <K>
  • Canonical example: (1 ∪ 4) + (3 ∪ 6) + (2 ∪ 5), K=12
  • 15 unit tests covering creation, evaluation, brute force, serialization, paper example
  • Paper entry with CeTZ expression tree diagram

Closes #552

Test plan

  • make check passes (fmt + clippy + test)
  • make paper compiles without errors
  • 15 unit tests all pass
  • Paper example verified: set = {6, 9, 12, 15}, 3 satisfying configs for K=12

🤖 Generated with Claude Code

Add the Integer Expression Membership problem: given a recursive integer
expression tree over union and Minkowski sum operations on singleton
positive integers, and a target K, decide whether K belongs to the set
represented by the expression. Includes model, tests, CLI support,
canonical example, and paper entry.

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

❌ Patch coverage is 98.17518% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.88%. Comparing base (f3837e2) to head (1fc3002).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/models/misc/integer_expression_membership.rs 95.65% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main     #814    +/-   ##
========================================
  Coverage   97.88%   97.88%            
========================================
  Files         643      645     +2     
  Lines       70418    70692   +274     
========================================
+ Hits        68926    69196   +270     
- Misses       1492     1496     +4     

☔ 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 IntegerExpressionMembership

Structural Completeness

# Check Status
1 Model file exists PASS — src/models/misc/integer_expression_membership.rs is present
2 inventory::submit! present PASS — schema entry is registered in the model file
3 Serialize / Deserialize on model PASS — IntegerExpressionMembership derives both
4 Problem trait impl PASS — impl Problem for IntegerExpressionMembership is present
5 Aggregate value is present PASS — type Value = Or
6 #[cfg(test)] + #[path = ...] test link PASS — model links to src/unit_tests/models/misc/integer_expression_membership.rs
7 Test file exists PASS — dedicated test file is present
8 Test file has >= 3 test functions PASS — 15 test functions
9 Registered in misc/mod.rs PASS
10 Re-exported in models/mod.rs PASS
11 Variant registration exists PASS — crate::declare_variants! is present
12 CLI resolve_alias entry PASS — canonical-name resolution is registry-driven in problemreductions-cli/src/problem_name.rs, so no per-model hardcoded alias entry is required
13 CLI create support PASS — problemreductions-cli/src/commands/create.rs handles the model
14 Canonical model example registered PASS — example specs are exported through src/models/misc/mod.rs and src/example_db/model_builders.rs
15 Paper display-name entry PASS — docs/paper/reductions.typ adds the display name
16 Paper problem-def block PASS — docs/paper/reductions.typ adds a problem-def section
17 Blacklisted generated files committed PASS — none of the forbidden generated files are in the diff

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • evaluate() correctness: ISSUE — IntExpr::Sum uses unchecked u64 addition at src/models/misc/integer_expression_membership.rs:96-100. A valid-looking instance such as {"Sum":[{"Atom":18446744073709551615},{"Atom":1}]} panics under pred solve instead of producing a boolean result.
  • dims() correctness: OK — vec![2; num_union_nodes()] matches the stated binary choice semantics.
  • Size getter consistency: OK — expression_size, num_union_nodes, num_atoms, and expression_depth are present and consistent with the implementation.
  • Integer domain validation: ISSUE — the model and paper say atoms/targets are positive integers, but IntegerExpressionMembership::new and pred create accept zero-valued atoms/targets.
  • Future reduction compatibility: ISSUE — SubsetSum on main uses arbitrary-precision BigUint (src/models/misc/subset_sum.rs:7-8, src/models/misc/subset_sum.rs:53-57), so the planned SubsetSum -> IntegerExpressionMembership rule cannot be total if this model remains u64-based.

Issue Compliance

# Check Status
1 Problem name matches issue OK
2 Mathematical definition matches ISSUE — the issue/paper define singleton positive integers and K in NN^+, but the implementation accepts Atom(0) and target = 0
3 Problem framing matches OK — decision problem with Value = Or
4 Type parameters match OK — implementation matches the issue’s submitted schema, though the fixed-width choice creates the semantic issue above
5 Configuration space matches OK — one binary variable per union node
6 Feasibility check matches ISSUE — out-of-domain zero-valued instances are currently treated as valid inputs
7 Objective function matches OK — membership in the represented set
8 Complexity matches OK — variant metadata and CLI both expose 2^num_union_nodes

Summary

  • 17/17 structural checks passed
  • 6/8 issue-compliance checks passed
  • FAIL / ISSUE items:
    • Unchecked u64 addition makes pred solve panic on overflow instead of evaluating the instance safely.
    • The model accepts zero-valued atoms and target values even though the definition says positive integers only.
    • Using u64 here is incompatible with the existing arbitrary-precision SubsetSum model that the planned incoming reduction is supposed to map from.

Quality Check

Quality Review

Design Principles

  • DRY: OK — the implementation is self-contained and does not introduce obvious duplication beyond the canonical example mirrored across code/tests/paper.
  • KISS: OK — the core tree evaluator and CLI parser are straightforward.
  • HC/LC: OK — model logic stays in the model file, while CLI wiring stays in the CLI layer.

HCI (if CLI/MCP changed)

  • Error messages: ISSUE — problemreductions-cli/src/commands/create.rs:2358-2360 says --target must be a positive integer, but --target 0 is accepted.
  • Discoverability: OK — the feature appears in pred list, pred show, pred create --help, and pred create --example.
  • Consistency: OK — the new CLI branch follows the existing pred create pattern.
  • Least surprise: ISSUE — a user can create an overflow instance successfully, then hit a Rust panic during pred solve.
  • Feedback: OK — successful pred create, pred solve, and pred evaluate outputs are clear.

Test Quality

  • Naive test detection: ISSUE
    • src/unit_tests/models/misc/integer_expression_membership.rs has good happy-path coverage, but it does not test zero-valued atoms/targets despite the positive-integer contract.
    • The same test file does not cover large-value / overflow boundaries even though evaluation uses raw u64 addition.

Issues

Critical (Must Fix)

  • src/models/misc/integer_expression_membership.rs:36, src/models/misc/integer_expression_membership.rs:81-100, src/models/misc/integer_expression_membership.rs:151, problemreductions-cli/src/commands/create.rs:2358-2364
    The model is implemented with fixed-width u64 integers and unchecked addition. Repro:
    • pred create IntegerExpressionMembership --expression '{"Sum":[{"Atom":18446744073709551615},{"Atom":1}]}' --target 0 -o overflow.json
    • pred solve overflow.json --solver brute-force
      Result: panic at src/models/misc/integer_expression_membership.rs:99 (attempt to add with overflow). This is both a correctness bug and a representational mismatch with the existing arbitrary-precision SubsetSum model (src/models/misc/subset_sum.rs:7-8, src/models/misc/subset_sum.rs:53-57).

Important (Should Fix)

  • src/models/misc/integer_expression_membership.rs:35-36, src/models/misc/integer_expression_membership.rs:160-161, problemreductions-cli/src/commands/create.rs:2358-2364, docs/paper/reductions.typ:6554
    The implementation accepts Atom(0) and target = 0, even though the model definition and paper entry say the domain is positive integers. Repro:
    • pred create IntegerExpressionMembership --expression '{"Atom":0}' --target 0 -o zero.json
      This succeeds and serializes an out-of-domain instance, so the current CLI validation text is misleading as well.

Minor (Nice to Have)

  • None beyond the issues above.

Summary

  • Critical: fixed-width u64 arithmetic causes overflow panics and blocks a total SubsetSum -> IntegerExpressionMembership rule.
  • Important: zero-valued atoms/targets are accepted despite the positive-integer definition.
  • Important: tests miss both of the broken boundary cases above.

Agentic Feature Tests

Feature Test Report: problem-reductions

Date: 2026-03-29
Project type: CLI + library
Features tested: IntegerExpressionMembership model CLI flow
Profile: ephemeral
Use Case: verify that a downstream CLI user can discover, inspect, create, solve, and evaluate the new model from docs/help alone
Expected Outcome: the model is discoverable in the CLI, the canonical example works end-to-end, and invalid inputs fail cleanly according to the documented positive-integer semantics
Verdict: fail
Critical Issues: 1

Summary

Feature Discoverable Setup Works Expected Outcome Met Doc Quality
IntegerExpressionMembership CLI flow yes yes partial no partial

Per-Feature Details

IntegerExpressionMembership CLI flow

  • Role: downstream CLI user validating a newly added model.
  • Use Case: discover the model, inspect its schema, build the canonical example, solve it with brute force, and try a couple of malformed / boundary inputs.
  • What they tried:
    • pred list
    • pred show IntegerExpressionMembership
    • pred create --example IntegerExpressionMembership -o iem.json
    • pred solve iem.json --solver brute-force
    • pred evaluate iem.json --config 1,1,0
    • pred create IntegerExpressionMembership --expression '{"Sum":[{"Atom":1},{"Atom":2}]}' --target 3
    • pred create IntegerExpressionMembership --expression '{"Atom":0}' --target 0 -o zero.json
    • pred create IntegerExpressionMembership --expression '{"Sum":[{"Atom":18446744073709551615},{"Atom":1}]}' --target 0 -o overflow.json && pred solve overflow.json --solver brute-force
  • Discoverability: good. The model appears in pred list, pred show, and pred create --help.
  • Setup: good. No extra setup was required beyond the compiled workspace binary.
  • Functionality: the happy path works. The example model can be created, solved, and evaluated successfully. Two negative-path issues were confirmed: zero-valued instances are accepted, and the overflow instance crashes the solver.
  • Expected vs Actual Outcome: partial. The normal example flow works, but invalid/boundary inputs do not behave according to the documented positive-integer contract, and one of them crashes the CLI.
  • Blocked steps: none.
  • Friction points: docs/paper say positive integers, but the CLI currently accepts zero and only fails later on overflow with a panic.
  • Doc suggestions: once the implementation is corrected, document the accepted numeric domain explicitly in pred create --help / error text.

Expected vs Actual Outcome

  • Expected: happy-path example works, zero-valued inputs are rejected, large values are handled or rejected safely.
  • Actual: happy-path example works; zero-valued inputs are accepted; large u64 sums can panic during solving.

Issues Found

  • Critical / confirmed: overflow instance panics during pred solve (attempt to add with overflow at src/models/misc/integer_expression_membership.rs:99).
  • Important / confirmed: zero-valued atoms and target are accepted despite the positive-integer semantics described in code and paper.

Suggestions

  • Replace u64 with an arbitrary-precision integer type compatible with SubsetSum, or reject values that cannot be represented / summed safely before evaluation.
  • Enforce the positive-integer domain in both the constructor/deserialization path and pred create.
  • Add regression tests for zero-valued inputs and overflow/arbitrary-precision boundaries.

Generated by review-pipeline

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@isPANN isPANN mentioned this pull request Mar 29, 2026
3 tasks
- Use checked_add for u64 addition to prevent overflow panic (evaluates
  to Or(false) on overflow instead of crashing)
- Validate all Atom values > 0 and target > 0 in constructor
- Add CLI-side validation with clean error messages
- Add boundary tests: overflow safety, zero atom/target rejection

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

Resolve conflicts: keep both IntegerExpressionMembership (PR) and
KthLargestMTuple, SchedulingToMinimizeWeightedCompletionTime,
QuadraticDiophantineEquations, FeasibleBasisExtension (main) additions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@isPANN isPANN merged commit b4a2c62 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] IntegerExpressionMembership

3 participants