Detect when MIP objectives must move in discrete steps. Tighten the dual bound using this info.#1239
Open
chris-maes wants to merge 1 commit into
Open
Detect when MIP objectives must move in discrete steps. Tighten the dual bound using this info.#1239chris-maes wants to merge 1 commit into
chris-maes wants to merge 1 commit into
Conversation
chris-maes
added
non-breaking
📝 WalkthroughWalkthroughThis PR introduces objective-step tightening: a lattice-aware bounding technique that tightens branch-and-bound node bounds and LP cutoffs when the MIP objective has a periodic structure (step size and bias). The change threads a new ChangesObjective Step Tightening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1230-1237: The code computes a tightened lattice-aware lower bound
into node_ptr->lower_bound (inside the
original_lp_.objective_step/objective_is_integral branches) but the subsequent
fathom/branch decision still uses leaf_obj; update the branch/fathom comparisons
to use node_ptr->lower_bound instead of leaf_obj so decisions are driven by the
tightened bound (replace occurrences of leaf_obj in the fathom/branch checks
near the first block and also in the similar logic in the 1247-1273 section);
ensure you keep the same tolerance logic (settings_.integer_tol) and the
existing comparison direction when swapping to node_ptr->lower_bound.
- Around line 1354-1365: The current cutoff computation in the objective_step
and integral branches subtracts an extra 1 (via (k - 1) and -1) which drops the
next attainable lattice value when cutoff is off-lattice; to fix, compute the
lattice index k = std::floor((cutoff - bias) / step + settings_.integer_tol)
(for objective_step) and use lp_settings.cut_off = k * step + bias +
settings_.dual_tol (remove the "-1" adjustment), and in the
original_lp_.objective_is_integral fallback use lp_settings.cut_off =
std::floor(cutoff + settings_.integer_tol) + settings_.dual_tol (remove the
"-1"); update code around original_lp_.objective_step, lp_settings.cut_off,
settings_.integer_tol, settings_.dual_tol, original_lp_.objective_is_integral
and k accordingly.
In `@cpp/src/mip_heuristics/problem/problem.cu`:
- Around line 1498-1504: Guard against non-finite lower bounds before converting
to a rational when setting lattice bias: in the block that assigns step_r[j] and
bias_r[j] (references: step_r, bias_r, var_bounds, get_lower,
rat_t::from_double) check that the double lower = get_lower(var_bounds[j]) is
finite (e.g. std::isfinite(lower)); if finite set bias_r[j] =
rat_t::from_double(lower), otherwise set bias_r[j] to a safe default such as
rat_t::zero() (or skip bias initialization) to avoid NaN; apply the same
finite-check logic to the other occurrence that forms lattice bias (the
assignment around the 1685-1691 region) so both places avoid converting ±inf.
In `@cpp/src/mip_heuristics/problem/problem.cuh`:
- Line 332: The new member objective_step (type
cuopt::linear_programming::dual_simplex::objective_step_t<f_t>) is not being
copied by the custom copy constructors in problem.cu, causing copied Problem
instances to lose objective-step metadata; update the copy constructor(s) and
copy-assignment (in cpp/src/mip_heuristics/problem/problem.cu) to explicitly
initialize/assign objective_step from the source object (use the member
initializer list or copy assignment) so the objective_step state is preserved
when copying Problem instances.
In `@cpp/src/mip_heuristics/solver.cu`:
- Around line 297-305: The code forwards objective_step unconditionally but
deterministic node-solve paths still use worker.local_upper_bound + dual_tol
instead of the lattice/integral cutoff; either restrict the propagation/logging
to modes that support the new cutoff or apply the same cutoff math in the
deterministic solvers. Update the block that sets
branch_and_bound_problem.objective_step (and the CUOPT_LOG_INFO) to check the
solver mode (use the same mode flag used for non-deterministic runs) before
assigning/logging objective_step, or alternatively modify the deterministic
solve path that computes the cutoff (references: worker.local_upper_bound,
dual_tol, and the deterministic node-solve functions) to use
context.problem_ptr->get_objective_step() and its lattice/integral cutoff
calculation so deterministic runs observe identical cutoff behavior. Ensure the
chosen change keeps behavior consistent across modes and remove duplicate
hard-coded cutoff 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: Enterprise
Run ID: a738089d-fdd2-4825-88a9-a94c113912ab
📒 Files selected for processing (10)
cpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/dual_simplex/presolve.cppcpp/src/dual_simplex/presolve.hppcpp/src/dual_simplex/user_problem.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/problem/problem.cucpp/src/mip_heuristics/problem/problem.cuhcpp/src/mip_heuristics/solver.cu
Comment on lines
+1230
to
1237
| if (original_lp_.objective_step.has_step()) { | ||
| f_t step = original_lp_.objective_step.step_size; | ||
| f_t bias = original_lp_.objective_step.bias; | ||
| // Round up to next value on the lattice: k * step + bias >= leaf_obj | ||
| f_t k = std::ceil((leaf_obj - bias) / step - settings_.integer_tol); | ||
| node_ptr->lower_bound = k * step + bias; | ||
| } else if (original_lp_.objective_is_integral) { | ||
| node_ptr->lower_bound = std::ceil(leaf_obj - settings_.integer_tol); |
There was a problem hiding this comment.
Fathom against the tightened node bound, not the raw LP objective.
After Lines 1230-1237, node_ptr->lower_bound may already be snapped up to the incumbent lattice value, but the branch/fathom check still uses leaf_obj. That keeps branching nodes that cannot produce a strictly improving integer solution anymore (for example, UB = 2, step = 0.5, leaf_obj = 1.7 snaps to LB = 2.0 and still branches here). Drive this decision from the tightened bound instead.
Also applies to: 1247-1273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1230 - 1237, The
code computes a tightened lattice-aware lower bound into node_ptr->lower_bound
(inside the original_lp_.objective_step/objective_is_integral branches) but the
subsequent fathom/branch decision still uses leaf_obj; update the branch/fathom
comparisons to use node_ptr->lower_bound instead of leaf_obj so decisions are
driven by the tightened bound (replace occurrences of leaf_obj in the
fathom/branch checks near the first block and also in the similar logic in the
1247-1273 section); ensure you keep the same tolerance logic
(settings_.integer_tol) and the existing comparison direction when swapping to
node_ptr->lower_bound.
Comment on lines
+1354
to
+1365
| if (original_lp_.objective_step.has_step()) { | ||
| f_t step = original_lp_.objective_step.step_size; | ||
| f_t bias = original_lp_.objective_step.bias; | ||
| // Any improving feasible solution must have objective <= cutoff - step. | ||
| f_t k = std::floor((cutoff - bias) / step + settings_.integer_tol); | ||
| lp_settings.cut_off = (k - 1) * step + bias + settings_.dual_tol; | ||
| } else if (original_lp_.objective_is_integral) { | ||
| // If the objective is integral, any feasible solution should produce an upper bound that is | ||
| // (approximately) integral. We add a small tolerance and floor this value to get an integer, | ||
| // we then subtract 1, to stop simplex on problems that cannot improve the primal objective. | ||
| lp_settings.cut_off = | ||
| std::floor(cutoff + settings_.integer_tol) - 1 + settings_.dual_tol; |
There was a problem hiding this comment.
The cutoff formula drops one attainable value when cutoff is off-lattice.
(floor((cutoff - bias) / step + tol) - 1) * step + bias is only correct when cutoff is already on the objective lattice. If the incumbent is between lattice points—or just slightly below one because upper_bound_ stores the raw compute_objective(...) sum—you skip the next improving objective as well. The same off-by-one exists in the integral fallback.
Possible fix
if (original_lp_.objective_step.has_step()) {
f_t step = original_lp_.objective_step.step_size;
f_t bias = original_lp_.objective_step.bias;
- // Any improving feasible solution must have objective <= cutoff - step.
- f_t k = std::floor((cutoff - bias) / step + settings_.integer_tol);
+ // Largest lattice value strictly below `cutoff`.
+ f_t k = std::ceil((cutoff - bias) / step - settings_.integer_tol);
lp_settings.cut_off = (k - 1) * step + bias + settings_.dual_tol;
} else if (original_lp_.objective_is_integral) {
- lp_settings.cut_off =
- std::floor(cutoff + settings_.integer_tol) - 1 + settings_.dual_tol;
+ lp_settings.cut_off =
+ std::ceil(cutoff - settings_.integer_tol) - 1 + settings_.dual_tol;
} else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1354 - 1365, The
current cutoff computation in the objective_step and integral branches subtracts
an extra 1 (via (k - 1) and -1) which drops the next attainable lattice value
when cutoff is off-lattice; to fix, compute the lattice index k =
std::floor((cutoff - bias) / step + settings_.integer_tol) (for objective_step)
and use lp_settings.cut_off = k * step + bias + settings_.dual_tol (remove the
"-1" adjustment), and in the original_lp_.objective_is_integral fallback use
lp_settings.cut_off = std::floor(cutoff + settings_.integer_tol) +
settings_.dual_tol (remove the "-1"); update code around
original_lp_.objective_step, lp_settings.cut_off, settings_.integer_tol,
settings_.dual_tol, original_lp_.objective_is_integral and k accordingly.
Comment on lines
+1498
to
+1504
| for (i_t j = 0; j < n_vars; ++j) { | ||
| bool is_int = (var_types[j] == var_t::INTEGER); | ||
| bool is_impl_int = (var_flags[j] & (i_t)problem_t<i_t, f_t>::VAR_IMPLIED_INTEGER) != 0; | ||
| if (is_int || is_impl_int) { | ||
| step_r[j] = {1, 1}; | ||
| bias_r[j] = rat_t::from_double(get_lower(var_bounds[j])); | ||
| } |
There was a problem hiding this comment.
Guard non-finite lower bounds before lattice bias math.
Line 1503 and Line 1690 use lower bounds directly when forming lattice bias. If a bound is ±inf, objective-step metadata becomes invalid (NaN/undefined), which can corrupt pruning/tightening decisions.
💡 Suggested fix
--- a/cpp/src/mip_heuristics/problem/problem.cu
+++ b/cpp/src/mip_heuristics/problem/problem.cu
@@
for (i_t j = 0; j < n_vars; ++j) {
bool is_int = (var_types[j] == var_t::INTEGER);
bool is_impl_int = (var_flags[j] & (i_t)problem_t<i_t, f_t>::VAR_IMPLIED_INTEGER) != 0;
if (is_int || is_impl_int) {
step_r[j] = {1, 1};
- bias_r[j] = rat_t::from_double(get_lower(var_bounds[j]));
+ const auto lb = get_lower(var_bounds[j]);
+ if (std::isfinite(lb)) {
+ bias_r[j] = rat_t::from_double(lb);
+ } else {
+ // Keep integrality information but avoid non-finite bias.
+ bias_r[j] = {0, 1};
+ }
}
}
@@
if (all_obj_vars_integral) {
@@
std::vector<f_t> nonzero_coefs;
f_t bias = 0;
for (i_t i = 0; i < n_variables; ++i) {
if (h_obj_coefs[i] == 0) continue;
f_t coef = h_obj_coefs[i];
f_t lb = get_lower(h_var_bounds[i]);
+ if (!std::isfinite(lb)) {
+ // Cannot derive a reliable lattice offset from non-finite bounds.
+ objective_step = {};
+ return;
+ }
nonzero_coefs.push_back(coef);
bias += coef * lb;
}Also applies to: 1685-1691
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/mip_heuristics/problem/problem.cu` around lines 1498 - 1504, Guard
against non-finite lower bounds before converting to a rational when setting
lattice bias: in the block that assigns step_r[j] and bias_r[j] (references:
step_r, bias_r, var_bounds, get_lower, rat_t::from_double) check that the double
lower = get_lower(var_bounds[j]) is finite (e.g. std::isfinite(lower)); if
finite set bias_r[j] = rat_t::from_double(lower), otherwise set bias_r[j] to a
safe default such as rat_t::zero() (or skip bias initialization) to avoid NaN;
apply the same finite-check logic to the other occurrence that forms lattice
bias (the assignment around the 1685-1691 region) so both places avoid
converting ±inf.
| bool is_scaled_{false}; | ||
| bool preprocess_called{false}; | ||
| bool objective_is_integral{false}; | ||
| cuopt::linear_programming::dual_simplex::objective_step_t<f_t> objective_step; |
There was a problem hiding this comment.
Copy operations currently drop objective_step state.
Line 332 adds a persistent member, but the custom copy constructors in cpp/src/mip_heuristics/problem/problem.cu don’t initialize it, so copied problems silently reset objective-step tightening metadata.
💡 Suggested fix
--- a/cpp/src/mip_heuristics/problem/problem.cu
+++ b/cpp/src/mip_heuristics/problem/problem.cu
@@
problem_t<i_t, f_t>::problem_t(const problem_t<i_t, f_t>& problem_)
: original_problem_ptr(problem_.original_problem_ptr),
@@
preprocess_called(problem_.preprocess_called),
objective_is_integral(problem_.objective_is_integral),
+ objective_step(problem_.objective_step),
lp_state(problem_.lp_state),
@@
problem_t<i_t, f_t>::problem_t(const problem_t<i_t, f_t>& problem_,
const raft::handle_t* handle_ptr_)
: original_problem_ptr(problem_.original_problem_ptr),
@@
preprocess_called(problem_.preprocess_called),
objective_is_integral(problem_.objective_is_integral),
+ objective_step(problem_.objective_step),
lp_state(problem_.lp_state, handle_ptr),
@@
problem_t<i_t, f_t>::problem_t(const problem_t<i_t, f_t>& problem_, bool no_deep_copy)
: original_problem_ptr(problem_.original_problem_ptr),
@@
preprocess_called(problem_.preprocess_called),
objective_is_integral(problem_.objective_is_integral),
+ objective_step(problem_.objective_step),
lp_state(problem_.lp_state),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/mip_heuristics/problem/problem.cuh` at line 332, The new member
objective_step (type
cuopt::linear_programming::dual_simplex::objective_step_t<f_t>) is not being
copied by the custom copy constructors in problem.cu, causing copied Problem
instances to lose objective-step metadata; update the copy constructor(s) and
copy-assignment (in cpp/src/mip_heuristics/problem/problem.cu) to explicitly
initialize/assign objective_step from the source object (use the member
initializer list or copy assignment) so the objective_step state is preserved
when copying Problem instances.
Comment on lines
+297
to
+305
| if (context.settings.objective_step && context.problem_ptr->get_objective_step().has_step()) { | ||
| CUOPT_LOG_INFO("Objective step size %g, bias %g", | ||
| context.problem_ptr->get_objective_step().step_size, | ||
| context.problem_ptr->get_objective_step().bias); | ||
| } | ||
| branch_and_bound_problem.objective_is_integral = context.problem_ptr->is_objective_integral(); | ||
| if (context.settings.objective_step) { | ||
| branch_and_bound_problem.objective_step = context.problem_ptr->get_objective_step(); | ||
| } |
There was a problem hiding this comment.
Deterministic mode still misses the tightened LP-cutoff half of this feature.
This now forwards objective_step for every B&B run, but the deterministic node-solve paths still hard-code worker.local_upper_bound + dual_tol and never apply the new lattice/integral cutoff calculation. That makes the same setting change search behavior by determinism mode. Either gate this propagation/logging to the supported modes or thread the cutoff logic through the deterministic solvers too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/mip_heuristics/solver.cu` around lines 297 - 305, The code forwards
objective_step unconditionally but deterministic node-solve paths still use
worker.local_upper_bound + dual_tol instead of the lattice/integral cutoff;
either restrict the propagation/logging to modes that support the new cutoff or
apply the same cutoff math in the deterministic solvers. Update the block that
sets branch_and_bound_problem.objective_step (and the CUOPT_LOG_INFO) to check
the solver mode (use the same mode flag used for non-deterministic runs) before
assigning/logging objective_step, or alternatively modify the deterministic
solve path that computes the cutoff (references: worker.local_upper_bound,
dual_tol, and the deterministic node-solve functions) to use
context.problem_ptr->get_objective_step() and its lattice/integral cutoff
calculation so deterministic runs observe identical cutoff behavior. Ensure the
chosen change keeps behavior consistent across modes and remove duplicate
hard-coded cutoff logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR recognizes when the objective for a MIP can only move in discrete steps or increments.
Integer variables l_j <= x_j <= u_j, are represented as x_j = k * 1 + l_j, where k in {0, ..., u_j - l_j}
You can then use equality constraints to try put other continuous variables in the form: y_j = k * s_j + o_j. And eventually show that an objective containing a mix of integer and continuous variables must also be in the form: k * step + offset (for an integer k)
Using this allows you to improve your lower bound and stop early. For example, let's say objective has to be a multiple of 1/2. You have an upper bound of 2 and a lower bound of 1.7. You can declare optimality already, since the next best objective is 1.5 and your lower bound is already better than that.
This structure appears in 30/240 MIPLIB benchmark problems. So this is fairly common.
5 minute MIPLIB benchmark run on GH200: