Skip to content

Add a dual aggregation presolver for binary variables with a single lock#901

Open
aliceb-nv wants to merge 501 commits intoNVIDIA:release/26.04from
aliceb-nv:setppc-presolve
Open

Add a dual aggregation presolver for binary variables with a single lock#901
aliceb-nv wants to merge 501 commits intoNVIDIA:release/26.04from
aliceb-nv:setppc-presolve

Conversation

@aliceb-nv
Copy link
Copy Markdown
Contributor

@aliceb-nv aliceb-nv commented Feb 23, 2026

This PR adds a dual aggregation pass to Papilo to recognize cases where a binary variable $x$ has only a single lock preventing it from reaching its objective-favorable bound. When found, the pass attempts to substitute $x$ with a master binary variable $y$ present in the locking row.

This new pass attempts to perform a substitution of $x$ with another binary variable y in the row that locks $x$.

For this, a probing is performed to try to prove that y = 0 => x = 0. We do this by showing x=1 AND y=0 violates the single locking row's activity bounds.
We also prove that y = 1 => x = 1 by showing that the constraint is always satisfied when y = 1, and the objective is not worsened by setting x to 1.

If these two clauses are true, the subsitution x = y can be safely performed.
We also handle the reverse case of a single down-lock.

This pass runs in O(nnz) time.

With these changes, we solve neos-787933 at the root, thus adding +1 optimal in 10min runs.

Description

Issue

Checklist

  • I am familiar with the Contributing Guidelines.
  • Testing
    • New or existing tests cover these changes
    • Added tests
    • Created an issue to follow-up
    • NA
  • Documentation
    • The documentation is up to date with these changes
    • Added new documentation
    • NA

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 3, 2026

🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you!

If this is an "epic" issue, then please add the "epic" label to this issue.
If it is a PR and not ready for review, then please convert this to draft.
If you just want to switch off this notification, then use the "skip inactivity reminder" label.

2 similar comments
@github-actions
Copy link
Copy Markdown

🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you!

If this is an "epic" issue, then please add the "epic" label to this issue.
If it is a PR and not ready for review, then please convert this to draft.
If you just want to switch off this notification, then use the "skip inactivity reminder" label.

@github-actions
Copy link
Copy Markdown

🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you!

If this is an "epic" issue, then please add the "epic" label to this issue.
If it is a PR and not ready for review, then please convert this to draft.
If you just want to switch off this notification, then use the "skip inactivity reminder" label.

@rgsl888prabhu rgsl888prabhu changed the base branch from main to release/26.04 March 19, 2026 16:35
@aliceb-nv
Copy link
Copy Markdown
Contributor Author

/ok to test 4caf1f1

@aliceb-nv
Copy link
Copy Markdown
Contributor Author

/ok to test b1f7657

@chris-maes chris-maes added P1 P2 and removed P1 labels Mar 25, 2026
@aliceb-nv
Copy link
Copy Markdown
Contributor Author

/ok to test a2b498b

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
cpp/tests/mip/presolve_test.cpp (1)

186-215: Consider adding test for DOWN direction.

All current tests cover the UP direction (negative objective coefficient, single up-lock). The algorithm also handles the symmetric DOWN direction (positive objective coefficient, single down-lock). Adding a test case would improve coverage of the symmetric logic path.

Example test structure:

// x has one down-lock in a GEQ row. Prove y=1 => x=1 via activity.
//   min x                    // positive objective => prefers decrease
//   s.t.  -3x + 4y >= -1     // GEQ: positive coeff gives down-lock
//         x, y in {0,1}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cpp/tests/mip/presolve_test.cpp` around lines 186 - 215, Add a symmetric test
for the DOWN direction to cover the case where the objective coefficient is
positive and a single down-lock exists: create a new TEST (e.g.,
SingleLockDualAggregation.FavorableStateRejectsDown) that mirrors
FavorableStateRejects but uses a positive objective for x and a GEQ row with a
negative coeff on x / positive on y (e.g., -3x + 4y >= -1) so the algorithm
follows the down-lock path; instantiate the same presolver
SingleLockDualAggregation<double>, run it with papilo_harness_t::run, and assert
the same expectations (presolve status kUnchanged and zero reductions) to
validate the symmetric logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cpp/src/dual_simplex/bounds_strengthening.cpp`:
- Around line 214-221: The code currently reverts small tightenings (using
lb_updated/ub_updated and restoring new_lb=new_ub=old_*) before checking
infeasibility; instead compute tentative new_lb/new_ub from
lower_bounds[k]/upper_bounds[k] without rollback, perform the infeasibility test
new_lb > new_ub + settings.primal_tol first, and only after that apply the
small-change rollback using the 1e3 * settings.primal_tol threshold to decide
whether to keep the tightening for propagation (i.e., restore to old_lb/old_ub
only for propagation decisions, not before the feasibility check). Ensure you
update the places referencing new_lb/new_ub, lb_updated, ub_updated, old_lb,
old_ub, lower_bounds, upper_bounds, and settings.primal_tol accordingly.

---

Nitpick comments:
In `@cpp/tests/mip/presolve_test.cpp`:
- Around line 186-215: Add a symmetric test for the DOWN direction to cover the
case where the objective coefficient is positive and a single down-lock exists:
create a new TEST (e.g., SingleLockDualAggregation.FavorableStateRejectsDown)
that mirrors FavorableStateRejects but uses a positive objective for x and a GEQ
row with a negative coeff on x / positive on y (e.g., -3x + 4y >= -1) so the
algorithm follows the down-lock path; instantiate the same presolver
SingleLockDualAggregation<double>, run it with papilo_harness_t::run, and assert
the same expectations (presolve status kUnchanged and zero reductions) to
validate the symmetric logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5081af3c-47a9-46d4-afdb-c47d3bc5b87f

📥 Commits

Reviewing files that changed from the base of the PR and between b1f7657 and a2b498b.

📒 Files selected for processing (5)
  • cpp/src/dual_simplex/bounds_strengthening.cpp
  • cpp/src/mip_heuristics/presolve/single_lock_dual_aggregation.cpp
  • cpp/src/mip_heuristics/presolve/third_party_presolve.cpp
  • cpp/src/pdlp/termination_strategy/infeasibility_information.cu
  • cpp/tests/mip/presolve_test.cpp
✅ Files skipped from review due to trivial changes (1)
  • cpp/src/pdlp/termination_strategy/infeasibility_information.cu
🚧 Files skipped from review as they are similar to previous changes (1)
  • cpp/src/mip_heuristics/presolve/third_party_presolve.cpp

Comment on lines +214 to +221
if (lb_updated)
new_lb = std::max(new_lb, lower_bounds[k]);
else
new_lb = old_lb;
if (ub_updated)
new_ub = std::min(new_ub, upper_bounds[k]);
else
new_ub = old_ub;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Keep sub-threshold tightenings for the feasibility check.

Line 214 through Line 221 revert any change smaller than 1e3 * settings.primal_tol before Line 223 checks new_lb > new_ub + settings.primal_tol. That can miss a real contradiction when both deductions are small: e.g. the current pass computes new_lb = 6e-4 and new_ub = 4e-4; both deltas fall under the default 1e-3 cutoff, so the code restores the old bounds and returns feasible even though the variable is already infeasible by 2e-4 > primal_tol.

Please defer the rollback until after the infeasibility test, and use the larger threshold only for deciding whether to propagate another iteration.

Possible fix
       bool lb_updated = std::abs(new_lb - old_lb) > 1e3 * settings.primal_tol;
       bool ub_updated = std::abs(new_ub - old_ub) > 1e3 * settings.primal_tol;
-      if (lb_updated)
-        new_lb = std::max(new_lb, lower_bounds[k]);
-      else
-        new_lb = old_lb;
-      if (ub_updated)
-        new_ub = std::min(new_ub, upper_bounds[k]);
-      else
-        new_ub = old_ub;
+      if (lb_updated) { new_lb = std::max(new_lb, lower_bounds[k]); }
+      if (ub_updated) { new_ub = std::min(new_ub, upper_bounds[k]); }
 
       if (new_lb > new_ub + settings.primal_tol) {
         settings.log.debug(
           "Iter:: %d, Infeasible variable after update %d, %e > %e\n", iter, k, new_lb, new_ub);
         last_nnz_processed = nnz_processed;
         return false;
       }
+
+      if (!lb_updated) { new_lb = old_lb; }
+      if (!ub_updated) { new_ub = old_ub; }
As per coding guidelines, validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cpp/src/dual_simplex/bounds_strengthening.cpp` around lines 214 - 221, The
code currently reverts small tightenings (using lb_updated/ub_updated and
restoring new_lb=new_ub=old_*) before checking infeasibility; instead compute
tentative new_lb/new_ub from lower_bounds[k]/upper_bounds[k] without rollback,
perform the infeasibility test new_lb > new_ub + settings.primal_tol first, and
only after that apply the small-change rollback using the 1e3 *
settings.primal_tol threshold to decide whether to keep the tightening for
propagation (i.e., restore to old_lb/old_ub only for propagation decisions, not
before the feasibility check). Ensure you update the places referencing
new_lb/new_ub, lb_updated, ub_updated, old_lb, old_ub, lower_bounds,
upper_bounds, and settings.primal_tol accordingly.

@chris-maes chris-maes modified the milestones: 26.04, 26.06 Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change P2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants