Skip to content

Fix #515: [Model] RegisterSufficiency#816

Merged
isPANN merged 6 commits intomainfrom
515-register-sufficiency
Mar 29, 2026
Merged

Fix #515: [Model] RegisterSufficiency#816
isPANN merged 6 commits intomainfrom
515-register-sufficiency

Conversation

@isPANN
Copy link
Copy Markdown
Collaborator

@isPANN isPANN commented Mar 28, 2026

Summary

  • Add RegisterSufficiency decision problem (Garey & Johnson A11 PO1, SS19): given a DAG G=(V,A) and bound K, decide whether the computation can be evaluated using at most K registers
  • Value type Or, category misc, no type parameters
  • Complexity: num_vertices ^ 2 * 2 ^ num_vertices (Kessler 1998 DP)
  • 12 unit tests covering creation, evaluation (valid/invalid), brute force, serialization, paper example
  • CLI pred create RegisterSufficiency --arcs ... --bound K handler
  • Canonical example in example-db (7 vertices, 8 arcs, K=3)
  • Paper entry with CeTZ-free prose, bibliography (Sethi 1975, Sethi-Ullman 1970, Kessler 1998)

Associated rules: #782 (RegisterSufficiency to ILP, separate issue)

Test plan

  • make test passes (all tests green)
  • make clippy passes
  • make fmt passes
  • make paper compiles
  • Example-db tests pass (model_specs_are_optimal)
  • Brute force confirms K=2 is impossible for the issue example (minimum is exactly 3)

Closes #515

🤖 Generated with Claude Code

Add the Register Sufficiency decision problem (Garey & Johnson A11 PO1):
given a DAG and bound K, determine whether the computation can be
performed using at most K registers. Value type Or, category misc.

Includes model, unit tests (12), CLI create handler, canonical example,
and paper entry with bibliography references (Sethi 1975, Sethi-Ullman
1970, Kessler 1998).

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.88%. Comparing base (b4a2c62) to head (359758a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #816    +/-   ##
========================================
  Coverage   97.88%   97.88%            
========================================
  Files         645      647     +2     
  Lines       70692    70940   +248     
========================================
+ Hits        69196    69443   +247     
- Misses       1496     1497     +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 RegisterSufficiency

Structural Completeness

# Check Status
1 Model file exists PASS — src/models/misc/register_sufficiency.rs
2 inventory::submit! present PASS
3 Serialize / Deserialize derive PASS
4 Problem trait impl PASS
5 Aggregate value is present PASS — type Value = Or
6 Test link present PASS
7 Test file exists PASS — src/unit_tests/models/misc/register_sufficiency.rs
8 Test file has >= 3 tests PASS — 11 tests
9 Registered in src/models/misc/mod.rs PASS
10 Re-exported in src/models/mod.rs PASS
11 Variant registration exists PASS — crate::declare_variants! is present in src/models/misc/register_sufficiency.rs:216; the review packet's missing-declare_variants flag is a stale checker false positive
12 CLI alias resolution PASS — canonical-name resolution is registry-driven in problem_name.rs, so no manual alias entry is required
13 CLI create support PASS — problemreductions-cli/src/commands/create.rs:3932
14 Canonical model example registered PASS — canonical_model_example_specs() is chained into src/models/misc/mod.rs:184, and src/example_db/model_builders.rs consumes crate::models::misc::canonical_model_example_specs()
15 Paper display-name entry PASS — docs/paper/reductions.typ:179
16 Paper problem-def block PASS — docs/paper/reductions.typ:4103

Build Status

  • make test: PASS
  • make clippy: PASS
  • make paper: PASS (extra verification because the paper changed)

Semantic Review

  • evaluate() correctness: OK — simulate_registers() rejects non-permutations and dependency violations, and the issue example/manual CLI evaluation agree with the intended semantics.
  • dims() correctness: OK — configuration space is one position variable per vertex with domain size num_vertices.
  • Size getter consistency: OK — num_vertices() / num_arcs() match the declared complexity metadata.
  • Weight handling: N/A — unweighted model.

Issue Compliance

# Check Status
1 Problem name matches issue OK
2 Mathematical definition matches OK
3 Problem framing matches OK — decision problem with Or value
4 Type parameters match OK — none
5 Configuration space matches OK — one permutation-position variable per vertex
6 Feasibility check matches OK — invalid permutations / dependency orderings are rejected
7 Objective function matches OK — returns whether the ordering uses at most K registers
8 Complexity matches OK — num_vertices ^ 2 * 2 ^ num_vertices matches the linked issue's final complexity string

Summary

  • 16/16 structural checks passed
  • 8/8 issue compliance checks passed
  • The implementation packet's declare_variants completeness failure is a checker false positive, not a PR defect.

Quality Check

Quality Review

Design Principles

  • DRY: OK — the register simulation logic is centralized in simulate_registers() and reused consistently.
  • KISS: OK — the model and CLI creation path stay straightforward and avoid unnecessary abstraction.
  • HC/LC: OK — model logic, CLI parsing, and paper/example wiring remain separated.

HCI (CLI changed)

  • Error messages: OK — pred create RegisterSufficiency prints concrete parameter help when required flags are missing.
  • Discoverability: ISSUE — the paper advertises pred solve register-sufficiency.json, but the default solve path does not exist on main. Reproduced: pred solve /tmp/register-sufficiency.json fails with No reduction path from RegisterSufficiency to ILP.
  • Consistency: ISSUE — the documented solve command in docs/paper/reductions.typ:4112 does not match current CLI behavior; the working invocation is pred solve register-sufficiency.json --solver brute-force.
  • Least surprise: ISSUE — users can create and inspect the model successfully, then hit a hard failure on the next documented command.
  • Feedback: OK — the failing solve command includes a useful brute-force hint.

Test Quality

  • Naive test detection: ISSUE
    • The PR adds CLI create support in problemreductions-cli/src/commands/create.rs:3932 and documents a create/solve flow in docs/paper/reductions.typ:4110, but there is no RegisterSufficiency-specific CLI regression in problemreductions-cli/tests/cli_tests.rs covering that documented path. That gap allowed the broken default-solver command to ship.

Issues

Critical (Must Fix)

None.

Important (Should Fix)

  1. docs/paper/reductions.typ:4112 documents pred solve register-sufficiency.json, but that command fails on current main because RegisterSufficiency has no default reduction path to ILP yet. Either change the paper command to --solver brute-force or defer the plain pred solve example until the companion ILP rule lands.
  2. Add a CLI regression test for the documented RegisterSufficiency example flow (pred create --example RegisterSufficiency plus the intended solve command) in problemreductions-cli/tests/cli_tests.rs.

Minor (Nice to Have)

None.

Summary

  • The implementation is clean and the CLI create UX is good.
  • The only user-facing issue is the documented default-solver mismatch, plus the missing regression coverage that would have caught it.

Agentic Feature Tests

Feature Test Report: problem-reductions

Date: 2026-03-29
Project type: CLI tool / library
Features tested: RegisterSufficiency model
Profile: ephemeral
Use Case: create the documented example instance, inspect it, solve it, and verify the paper configuration from the CLI alone
Expected Outcome: the new model is discoverable in the catalog and the documented create/show/solve/evaluate flow works end-to-end
Verdict: fail
Critical Issues: 0

Summary

Feature Discoverable Setup Works Expected Outcome Met Doc Quality
RegisterSufficiency yes yes partial partial documented solve command is wrong

Per-Feature Details

RegisterSufficiency

  • Role: downstream CLI user exploring a newly added model
  • Use Case: create the documented example instance, inspect it, solve it, and verify the paper configuration
  • What they tried:
    • pred list — PASS, model appears in the catalog
    • pred show RegisterSufficiency — PASS, fields and complexity render correctly
    • pred create --example RegisterSufficiency -o /tmp/register-sufficiency.json — PASS
    • pred solve /tmp/register-sufficiency.json — FAIL (confirmed)
    • pred solve /tmp/register-sufficiency.json --solver brute-force — PASS
    • pred evaluate /tmp/register-sufficiency.json --config 0,1,2,3,5,4,6 — PASS
  • Discoverability: Good; the model is visible in pred list and pred show.
  • Setup: No extra setup required once the CLI is built.
  • Functionality: Model creation, catalog display, brute-force solving, and explicit evaluation all work. The documented default solve command does not.
  • Expected vs Actual Outcome: Partial. The feature itself works, but the paper's advertised solve path fails until the user adds --solver brute-force.
  • Blocked steps: None.
  • Friction points: The paper implies pred solve should succeed directly, but the CLI falls back to ILP path search and there is no such path yet.
  • Doc suggestions: Update the paper command or wait until the companion RegisterSufficiency -> ILP rule exists.

Expected vs Actual Outcome

  • Expected: the paper example could be created, solved, and evaluated from docs alone.
  • Actual: create/show/evaluate succeed; solve succeeds only with --solver brute-force.

Issues Found

  • Important: the documented pred solve command fails on current main.
  • Important: there is no CLI regression covering the documented RegisterSufficiency example flow.

Suggestions

  • Change the paper command to pred solve register-sufficiency.json --solver brute-force.
  • Add CLI integration coverage for the example flow.

Generated by review-pipeline

# Conflicts:
#	problemreductions-cli/src/commands/create.rs
#	src/models/misc/mod.rs
#	src/models/mod.rs
@isPANN isPANN mentioned this pull request Mar 29, 2026
3 tasks
isPANN and others added 2 commits March 29, 2026 21:22
RegisterSufficiency has no ILP reduction path yet, so the default
solver fails. Use --solver brute-force consistent with other models
lacking an ILP path (e.g., PathConstrainedNetworkFlow, RootedTreeArrangement).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers the documented create flow with --arcs, --bound, --num-vertices
flags and verifies the output JSON structure.

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 RegisterSufficiency (PR) and
IntExpr, IntegerExpressionMembership, KthLargestMTuple,
SchedulingToMinimizeWeightedCompletionTime (main) in imports and specs.

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

3 participants