Skip to content

Fix #123: [Rule] SteinerTree to ILP#708

Merged
isPANN merged 10 commits intomainfrom
issue-123
Mar 20, 2026
Merged

Fix #123: [Rule] SteinerTree to ILP#708
isPANN merged 10 commits intomainfrom
issue-123

Conversation

@GiggleLiu
Copy link
Copy Markdown
Contributor

Summary

Add the implementation plan and execution branch for the SteinerTree -> ILP reduction from issue #123.

Fixes #123

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.57%. Comparing base (f3204d5) to head (077f14e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #708    +/-   ##
========================================
  Coverage   97.56%   97.57%            
========================================
  Files         379      381     +2     
  Lines       47506    47649   +143     
========================================
+ Hits        46350    46494   +144     
+ Misses       1156     1155     -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 Author

Implementation summary for #123

Changes:

  • added ReductionSteinerTreeToILP with a rooted multi-commodity flow ILP formulation in src/rules/steinertree_ilp.rs
  • registered the new rule and canonical example wiring in src/rules/mod.rs
  • added focused rule/unit coverage for reduction shape, round-trip solving, extraction, and negative-weight rejection
  • documented the reduction in the paper and added supporting bibliography entries
  • removed the temporary plan file after implementation per pipeline requirements

Deviation from plan:

  • added a nonnegative-edge-weight guard because the formulation is only sound for nonnegative weights even though the source model stores weights as i32

Open questions:

  • none at the moment

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Agentic Review Report

Structural Check

Structural Review: rule SteinerTree -> ILP

Structural Completeness

# Check Status
1 Rule file exists PASS — src/rules/steinertree_ilp.rs
2 #[reduction(...)] macro present PASS — src/rules/steinertree_ilp.rs
3 ReductionResult impl present PASS — src/rules/steinertree_ilp.rs
4 ReduceTo impl present PASS — src/rules/steinertree_ilp.rs
5 #[cfg(test)] + #[path = ...] test link PASS — src/rules/steinertree_ilp.rs
6 Test file exists PASS — src/unit_tests/rules/steinertree_ilp.rs
7 Closed-loop test present PASS — test_steinertree_to_ilp_closed_loop in src/unit_tests/rules/steinertree_ilp.rs
8 Registered in rules/mod.rs PASS — src/rules/mod.rs
9 Canonical rule example registered PASS — canonical_rule_example_specs in src/rules/steinertree_ilp.rs, collected from src/rules/mod.rs
10 Example-db lookup coverage PASS — existing src/unit_tests/example_db.rs coverage passed locally, including exact direct-rule coverage
11 Paper reduction-rule entry PASS — docs/paper/reductions.typ
12 File whitelist FAIL — the deterministic whitelist flags docs/paper/references.bib as outside the current rule whitelist; manual inspection suggests this is a whitelist/tooling limitation rather than an unsafe PR change
13 Blacklisted auto-generated files PASS — none present in the diff

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • extract_solution: ISSUE — src/rules/steinertree_ilp.rs:36-37 returns the raw y_e prefix without pruning. Because the ILP only enforces f^t_(u,v) <= y_e (src/rules/steinertree_ilp.rs:95-105), optimal solutions on zero-weight instances can keep extra selected edges that carry no flow. Reproduced with pred create SteinerTree --graph 0-1,1-2,0-2 --edge-weights 0,0,0 --terminals 0,2: brute force returns Valid(0) with [0,0,1], but pred solve via ILP returns Invalid with [1,1,1].
  • Overhead accuracy: OK — canonical counts and exported reduction metadata both report 35 variables and 38 constraints, matching m + 2m(k-1) and n(k-1) + 2m(k-1).
  • Example quality: OK — the paper example and canonical bundle line up with the implementation.
  • Paper proof/extraction alignment: ISSUE — the paper correctly argues that pruning can recover a Steiner tree for nonnegative weights, but the implementation never performs that pruning. docs/paper/reductions.typ:4867-4871

Issue Compliance (if linked issue found)

# Check Status
1 Source/target match issue OK
2 Reduction construction matches OK
3 Solution extraction matches ISSUE — direct y_e extraction is not sound for zero-weight optima; pruning is required or the precondition must be tightened to strictly positive weights
4 Correctness preserved ISSUE — confirmed counterexample above produces an invalid extracted Steiner tree from a valid optimal ILP solution
5 Overhead expressions match OK
6 Example matches OK

Summary

  • 12/13 structural checks passed; the remaining failure is the deterministic whitelist flag on docs/paper/references.bib
  • 4/6 issue-compliance checks passed
  • Must fix: prune the selected y_e subgraph during extraction (or otherwise enforce tree-minimality) before claiming support for nonnegative weights

Quality Check

Quality Review

Design Principles

  • DRY: OK — the reduction is self-contained and does not introduce obvious duplication.
  • KISS: ISSUE — the extraction path is too optimistic about the y_e prefix already being a tree. src/rules/steinertree_ilp.rs:36-37
  • HC/LC: OK — problem logic, registration, tests, and paper edits are kept in the expected locations.

HCI (if CLI/MCP changed)

  • Not applicable — CLI/MCP files unchanged.

Test Quality

  • Naive test detection: ISSUE
    • src/unit_tests/rules/steinertree_ilp.rs:41-89 only exercises strictly positive-weight cases plus a negative-weight panic. It misses the zero-weight counterexample where ILPSolver::solve_reduced returns an invalid Steiner tree.

Issues

Critical (Must Fix)

  • src/rules/steinertree_ilp.rs:36-37: extraction returns raw edge selectors instead of pruning them to a Steiner tree. On zero-weight instances, pred solve via ILP can return Invalid even though brute force finds a valid optimum of the same cost.

Important (Should Fix)

  • src/unit_tests/rules/steinertree_ilp.rs:41-89: add a regression covering zero-weight edges after fixing extraction.

Minor (Nice to Have)

  • none.

Summary

  • Confirmed correctness bug in extraction for zero-weight/nonnegative instances.
  • Coverage misses the edge case that exposes it.

Agentic Feature Tests

Feature tested: SteinerTree -> ILP CLI path

Discoverability / Setup

  • target/debug/pred list --rules shows SteinerTree/SimpleGraph/i32 -> ILP/bool.
  • target/debug/pred show SteinerTree lists the outgoing reduction to ILP/bool.
  • target/debug/pred create --example SteinerTree and target/debug/pred create --example SteinerTree --to ILP --example-side target both succeed.

Functionality

  • Canonical happy path: PASS
    • pred solve steiner.json returns Valid(6) via reduced_to = ILP.
    • pred reduce steiner.json --to ILP produces an ILP bundle whose target has 35 variables and 38 constraints.
    • pred solve steiner_to_ilp.bundle.json returns Valid(6) and exposes the intermediate ILP witness.
  • Adversarial zero-weight path: FAIL
    • Reproduced on pred create SteinerTree --graph 0-1,1-2,0-2 --edge-weights 0,0,0 --terminals 0,2.
    • pred solve ... --solver brute-force => Valid(0) with [0,0,1].
    • pred solve ... (ILP path) => Invalid with [1,1,1].
    • Classification: confirmed.
    • Severity: critical.
    • Recommended fix: prune the selected edge subgraph before returning the source configuration, or add a secondary tie-break / constraints so the ILP selector set is itself a tree.

Blocked steps

  • none.

Doc suggestions

  • Once extraction is fixed, mention the nonnegative-weight nuance explicitly in the paper/CLI docs, or tighten the rule to strictly positive weights if pruning is intentionally omitted.

Generated by review-pipeline

isPANN and others added 4 commits March 21, 2026 02:39
Zero-weight edges can cause the ILP to return non-tree optimal solutions
(redundant zero-cost cycles). Following SCIP-Jack's convention, require
strictly positive weights — zero-weight edges should be contracted before
reduction. Added regression test for zero-weight rejection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align the paper's construction and correctness proof with the
implementation's tightened precondition.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@isPANN isPANN mentioned this pull request Mar 20, 2026
isPANN and others added 2 commits March 21, 2026 03:10
Entries booth1975, booth1976, lawler1972, eppstein1992, chopra1996,
kou1977, boothlueker1976 were duplicated during merge-with-main.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ction

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@isPANN isPANN merged commit ca2110e into main Mar 20, 2026
5 checks passed
@GiggleLiu GiggleLiu deleted the issue-123 branch April 12, 2026 00:50
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.

[Rule] SteinerTree to ILP

2 participants