Detect when MIP objectives must move in discrete steps. Tighten the dual bound using this info.#1239
Detect when MIP objectives must move in discrete steps. Tighten the dual bound using this info.#1239chris-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
| 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.
There was a problem hiding this comment.
Maybe we should apply this directly to the leaf_obj and then update the node_ptr->lower_bound.
There was a problem hiding this comment.
@nguidotti, that's a cleaner approach. Snapping leaf_obj in-place immediately after the update_pseudo_costs and graphviz calls (which should continue to reflect the raw LP objective) means every downstream use — including the handle_integer_solution path and the fathom check — automatically picks up the tightened value without touching the comparison expressions:
f_t leaf_obj = compute_objective(leaf_problem, leaf_solution.x);
policy.graphviz(search_tree, node_ptr, "lower bound", leaf_obj); // raw LP obj
policy.update_pseudo_costs(node_ptr, leaf_obj); // raw LP obj
// Snap leaf_obj to the feasible lattice before any bound/fathom decisions
if (original_lp_.objective_step.has_step()) {
f_t step = original_lp_.objective_step.step_size;
f_t bias = original_lp_.objective_step.bias;
f_t k = std::ceil((leaf_obj - bias) / step - settings_.integer_tol);
leaf_obj = k * step + bias;
} else if (original_lp_.objective_is_integral) {
leaf_obj = std::ceil(leaf_obj - settings_.integer_tol);
}
node_ptr->lower_bound = leaf_obj; // tightened
// ... num_frac == 0 check, fathom check — all use tightened leaf_objThe only ordering constraint to preserve is that graphviz and update_pseudo_costs are called before the snap, which they already are. This resolves the original concern more elegantly. 🐇
🧠 Learnings used
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 922
File: cpp/src/branch_and_bound/pseudo_costs.cpp:85-87
Timestamp: 2026-03-03T15:03:14.218Z
Learning: In cuOpt, the dual simplex method is dual-feasible throughout execution in the files under cpp/src/dual_simplex/ and cpp/src/branch_and_bound/. When a solver terminates early with status ITERATION_LIMIT, the current objective value is still a valid (lower) bound for minimization, though looser than the optimum. Treat ITERATION_LIMIT results as safe for strong branching bound tightening, fixings, and cutoff logic in these modules. Apply this guideline to all cpp/src/branch_and_bound and related dual_simplex implementations (i.e., all .cpp files under cpp/src/branch_and_bound and cpp/src/dual_simplex).
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 986
File: cpp/src/branch_and_bound/branch_and_bound.cpp:8-8
Timestamp: 2026-03-23T11:33:23.998Z
Learning: In this repo (NVIDIA/cuopt), treat nvcc as the supported CUDA toolchain; clang-based compilation/support is not required and may fail/break. During code reviews, do NOT request code changes or add blocking comments for errors that appear only under clang (e.g., header-resolution failures such as 'utilities/determinism_log.hpp not found')—these can be toolchain-related rather than real source issues.
Learnt from: bdice
Repo: NVIDIA/cuopt PR: 1035
File: cpp/tests/utilities/base_fixture.hpp:29-47
Timestamp: 2026-04-19T16:49:17.616Z
Learning: In NVIDIA/cuopt (and the upstream rapidsai/rmm) after the RMM → CCCL migration, the rmm::mr resource adaptors (e.g., rmm::mr::pool_memory_resource and rmm::mr::binning_memory_resource) are now owning: they take/hold their upstream resources by value instead of non-owning references. Therefore, direct construction of adaptor chains from temporaries/local values (e.g., pool_memory_resource(make_async(), size)) is safe and should NOT be flagged as potential dangling/lifetime bugs. Also, rmm::mr::make_owning_wrapper is no longer needed/available for this owning design, so do not suggest it in this codepath.
There was a problem hiding this comment.
Probably we should distinguish between the objective at the node (leaf_obj) and the lower bound coming from integrality or objective steps. To me this suggests that we use node_ptr->lower_bound rather than leaf_obj in the fathom check.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| 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.
There was a problem hiding this comment.
How could cutoff not be on the objective lattice? Cut off is equal to the objective of the best integer feasible solution.
There was a problem hiding this comment.
@chris-maes, you're absolutely right — I missed that. Since upper_bound_ is always the objective of the best integer feasible solution found so far, and integer feasible solutions necessarily have objectives on the lattice (that's precisely what the lattice detection establishes), cutoff is always a lattice point. So:
floor((cutoff - bias) / step + tol)gives the exact indexkwithk * step + bias = cutoff, and(k - 1) * step + biasis the largest lattice value strictly belowcutoff— which is the correct tightest cutoff.
The same reasoning holds for the objective_is_integral fallback: the incumbent is integral, so floor(cutoff + tol) - 1 = cutoff - 1 is exactly right.
My concern was unfounded. Sorry for the noise!
🧠 Learnings used
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 922
File: cpp/src/branch_and_bound/pseudo_costs.cpp:85-87
Timestamp: 2026-03-03T15:03:14.218Z
Learning: In cuOpt, the dual simplex method is dual-feasible throughout execution in the files under cpp/src/dual_simplex/ and cpp/src/branch_and_bound/. When a solver terminates early with status ITERATION_LIMIT, the current objective value is still a valid (lower) bound for minimization, though looser than the optimum. Treat ITERATION_LIMIT results as safe for strong branching bound tightening, fixings, and cutoff logic in these modules. Apply this guideline to all cpp/src/branch_and_bound and related dual_simplex implementations (i.e., all .cpp files under cpp/src/branch_and_bound and cpp/src/dual_simplex).
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 986
File: cpp/src/branch_and_bound/branch_and_bound.cpp:8-8
Timestamp: 2026-03-23T11:33:23.998Z
Learning: In this repo (NVIDIA/cuopt), treat nvcc as the supported CUDA toolchain; clang-based compilation/support is not required and may fail/break. During code reviews, do NOT request code changes or add blocking comments for errors that appear only under clang (e.g., header-resolution failures such as 'utilities/determinism_log.hpp not found')—these can be toolchain-related rather than real source issues.
Learnt from: bdice
Repo: NVIDIA/cuopt PR: 1035
File: cpp/tests/utilities/base_fixture.hpp:29-47
Timestamp: 2026-04-19T16:49:17.616Z
Learning: In NVIDIA/cuopt (and the upstream rapidsai/rmm) after the RMM → CCCL migration, the rmm::mr resource adaptors (e.g., rmm::mr::pool_memory_resource and rmm::mr::binning_memory_resource) are now owning: they take/hold their upstream resources by value instead of non-owning references. Therefore, direct construction of adaptor chains from temporaries/local values (e.g., pool_memory_resource(make_async(), size)) is safe and should NOT be flagged as potential dangling/lifetime bugs. Also, rmm::mr::make_owning_wrapper is no longer needed/available for this owning design, so do not suggest it in this codepath.
| 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.
There was a problem hiding this comment.
Where does an infinite lower bound lead to a NaN?
There was a problem hiding this comment.
@chris-maes — good question. The NaN path is clearest at Location 2 (compute_objective_step):
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]); // could be -inf
nonzero_coefs.push_back(coef);
bias += coef * lb; // → bias becomes -inf or +inf
}
// ...
objective_step.bias = std::fmod(bias, g); // std::fmod(±inf, g) == NaN (IEEE 754)std::fmod(±∞, finite_nonzero) returns NaN per the C++ / IEEE 754 standard, so if any objective variable has an unbounded lower bound, objective_step.bias becomes NaN and will corrupt the pruning/tightening logic downstream.
At Location 1 (propagate_lattice) the direct issue is slightly different — not NaN but undefined behavior: rat_t::from_double calls rational_approximation, which does (int64_t)std::floor(±inf), and casting infinity to int64_t is UB in C++. Depending on the platform this gives garbage integer values rather than NaN, but the downstream lattice propagation results would still be incorrect.
In short, the NaN you were asked about is real and concrete at Location 2 via std::fmod.
| 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.
| 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.
There was a problem hiding this comment.
Yep. We will leave moving this into deterministic mode as a TODO.
There was a problem hiding this comment.
@chris-maes, understood — deferring deterministic-mode support for objective_step tightening makes sense for now. Would you like me to open a GitHub issue to track this TODO so it doesn't get lost? I can file it with a reference back to this PR.
🧠 Learnings used
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 899
File: cpp/src/mip_heuristics/solve.cu:169-193
Timestamp: 2026-02-24T14:37:06.700Z
Learning: In cuOpt's MIP solver, the sign of objective_scaling_factor encodes the problem direction: >= 0 indicates minimization; < 0 indicates maximization (internally converted to minimization by negating the objective). Therefore, during code reviews, treat the sign of objective_scaling_factor as the source of truth for problem direction and use it to set appropriate 'no bound' values for callbacks accordingly.
| // Iteratively propagate | ||
| bool any_discovered = false; | ||
| bool changed = true; | ||
| while (changed) { |
There was a problem hiding this comment.
We can keep changed per equation right? That way we only propagate on the equations that contain variables contained in changed equations in the previous iteration.
| // Internally uses rational arithmetic (int64_t numerator/denominator pairs) to avoid | ||
| // floating-point GCD issues. | ||
| template <typename i_t, typename f_t, typename f_t2> | ||
| static bool propagate_lattice(i_t n_vars, |
There was a problem hiding this comment.
Could you briefly explain the lattice propagation logic?
I think we can also divide this function a bit into logical pieces. I think we can also move the rat_t out.
nguidotti
left a comment
nguidotti
left a comment
There was a problem hiding this comment.
Looks good to me! We can also use this for bound propagation in the future. Just a minor comment: if we have a dependency on boost, maybe we should use boost/rational.hpp instead of our own implementation.
| 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.
Maybe we should apply this directly to the leaf_obj and then update the node_ptr->lower_bound.
4 participants
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: