Recursive subMIP#1482
Draft
nguidotti wants to merge 28 commits into
Draft
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
cb9557d to
aefe31a
Compare
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…bmip (and restarted) problems. Papilo can now be applied to an user_problem_t inplace. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
# Conflicts: # cpp/src/mip_heuristics/diversity/lns/rins.cu # cpp/src/mip_heuristics/diversity/recombiners/sub_mip.cuh
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…e old warm start code. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…/root change diving procedure when the RINS neighbourhood is not large enough Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…ation count for the root relaxation (it should reflect the number of simplex iterations). Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…. this enable the use of diving heuristics for RINS. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…f the neighbourhood is too loose or it already found an improving solution. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Contributor
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis PR adds Sub-MIP and RINS support to branch-and-bound, including a new presolver wrapper and PaPILO-based crush/uncrush implementations, pseudo-cost warm starting, worker pool restructuring, and renamed iteration accounting (LP iterations to simplex iterations). It also adjusts initial-solution handling, adds tests/datasets, and includes minor unrelated fixes. ChangesSub-MIP/RINS and Presolve Crush Support
Estimated code review effort: 5 (Critical) | ~120 minutes Unrelated Standalone Fixes
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/dual_simplex/simplex_solver_settings.hpp (1)
69-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
bnb_iteration_limitdefault uses 32-biti_tmax instead of its ownint64_twidth.
bnb_iteration_limitis declaredint64_tbut initialized withstd::numeric_limits<i_t>::max()(i.e.intmax ≈ 2.1B) rather thanstd::numeric_limits<int64_t>::max(). This silently caps the "no limit" sentinel far below what the 64-bit field can represent, unlikeiteration_limitabove it (which is correctly typedi_t). For very long-running/deeply recursive sub-MIP solves accumulating simplex iterations, this could trigger a prematureITERATION_LIMITstatus well before an actual user-configured limit would be reached.🔧 Proposed fix
- bnb_iteration_limit(std::numeric_limits<i_t>::max()), + bnb_iteration_limit(std::numeric_limits<int64_t>::max()),🤖 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/dual_simplex/simplex_solver_settings.hpp` around lines 69 - 73, The default initialization for bnb_iteration_limit in simplex_solver_settings should use the full 64-bit sentinel instead of i_t max, since the member is int64_t and should match its declared width. Update the constructor/initializer in simplex_solver_settings so bnb_iteration_limit is initialized with the int64_t maximum value, keeping it consistent with the other limit fields and avoiding an unintended 32-bit cap.
🧹 Nitpick comments (2)
cpp/src/branch_and_bound/pseudo_costs.hpp (1)
130-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
operator=doesn't propagate thewarm_startflag, so copies of a warm-started object silently lose their data on the nextresize().
operator=copiespseudo_cost_sum_*/pseudo_cost_num_*but notwarm_start. Any copy of a warm-startedpseudo_costs_t(e.g. via the copy constructor at Line 125) will havewarm_start == false, so a laterresize()call will zero out the data that was just copied.♻️ Proposed fix
pseudo_costs_t& operator=(const pseudo_costs_t& other) { if (this != &other) { this->AT = other.AT; this->pdlp_warm_cache = other.pdlp_warm_cache; this->pseudo_cost_num_down = other.pseudo_cost_num_down; this->pseudo_cost_num_up = other.pseudo_cost_num_up; this->pseudo_cost_sum_down = other.pseudo_cost_sum_down; this->pseudo_cost_sum_up = other.pseudo_cost_sum_up; + this->warm_start = other.warm_start; } return *this; }Also applies to: 176-198
🤖 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/pseudo_costs.hpp` around lines 130 - 141, The copy assignment in pseudo_costs_t::operator= is missing the warm_start state, so copied warm-started instances lose their preserved pseudo-cost data before a later resize() call. Update pseudo_costs_t::operator= to copy warm_start alongside AT, pdlp_warm_cache, and the pseudo_cost_* fields, and make sure the same state is preserved consistently in the copy-construction path that shares this assignment behavior.cpp/src/dual_simplex/presolve.cpp (1)
688-767: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd defensive assertions for
convert_simplex_problem's implicit "one slack per row, non-dualized" precondition.The function silently assumes
new_slacks.size() == simplex_problem.num_rows(every row has exactly one slack/artificial column) and thatsimplex_problemwas not produced viaconvert_user_problem's dualization branch. If either assumption is violated, rows are left at the default'E'/rhs=0initialization instead of failing, silently corrupting the recovereduser_problem_t.Suggested guard
const i_t num_slacks = static_cast<i_t>(new_slacks.size()); + assert(num_slacks == m && + "convert_simplex_problem requires exactly one slack/artificial column per row"); const i_t new_n = n - num_slacks;🤖 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/dual_simplex/presolve.cpp` around lines 688 - 767, Add defensive checks in convert_simplex_problem to enforce its implicit preconditions before rebuilding user_problem_t: verify that new_slacks contains exactly one slack/artificial column per row and that the simplex problem was not dualized. Use the existing symbols simplex_problem.num_rows, new_slacks, and the convert_simplex_problem setup to assert or fail fast when these assumptions are not met, so the row_sense/rhs reconstruction cannot silently leave rows at the default equality state.
🤖 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 2151-2331: The RINS neighborhood LP solves in `rins()` are not
being counted toward `exploration_stats_.total_simplex_iters`, so the global
iteration budget can be exceeded. Update the `rins()` neighborhood-fixing loop
to merge the local `rins_stats` simplex iterations back into
`exploration_stats_` (or accumulate them once after the loop), while preserving
any intra-loop checks that rely on `rins_stats.total_simplex_iters` for
`solve_node_lp`.
- Around line 2401-2409: The RINS sub-MIP setup in branch_and_bound.cpp can
compute a NaN fixrate when integer_list is empty and num_integers is 0. Add an
early guard in the logic around has_submip / num_var_fixed so the code skips
fixrate calculation and stats updates when there are no integer variables to
fix, or otherwise ensure the sub-MIP is not created in that case. Make the same
safeguard wherever this branch is duplicated so submip_stats_.save_success,
save_infeasible, and submip_get_max_fixrate never receive a 0/0-derived value.
- Around line 1743-1747: The `bnb_iteration_limit` early-exit path in
`branch_and_bound.cpp` is missing the `--exploration_stats_.nodes_being_solved;`
cleanup that the nearby `TIME_LIMIT` and `NODE_LIMIT` branches already perform.
Update the `branch_and_bound` loop’s `bnb_iteration_limit` check so it
decrements `nodes_being_solved` before `stack.push_front(node_ptr)` and `break`,
keeping the counter consistent with `node_ptr` re-queued for later work and
preventing skew in `exploration_stats_.nodes_explored +
exploration_stats_.nodes_being_solved`.
- Around line 2177-2212: The sub-MIP settings are being built from a fresh
default instance, so recursive solves ignore parent/custom values like
iteration_limit_ratio and related submip options. Seed simplex_solver_settings_t
submip_settings from settings_.submip_settings first, then override only the
recursion-specific fields in branch_and_bound.cpp (for example node_limit,
time_limit, level, enable_rins, logging, and other sub-MIP-only toggles). Make
sure the bnb_iteration_limit calculation uses the copied parent setting rather
than the defaulted child value.
In `@cpp/src/branch_and_bound/pseudo_costs.hpp`:
- Around line 176-198: The warm-start mapping in pseudo_costs_t::warm_start_from
only checks that reduced_to_original[k] is non-negative, but it can still index
past the parent pseudo-cost arrays. Add an upper-bound assertion for orig before
reading parent.pseudo_cost_num_up, parent.pseudo_cost_sum_up,
parent.pseudo_cost_num_down, and parent.pseudo_cost_sum_down, using the parent
vectors’ size as the limit. Keep the existing lower-bound assert and place the
new check in the same loop so invalid reduced_to_original entries are caught
before any out-of-bounds access.
In `@cpp/src/branch_and_bound/worker.hpp`:
- Around line 178-197: `next_diving_heuristic()` can crash when
`diving_heuristics` is empty because it performs a modulo by
`diving_heuristics.size()`. Update the worker setup path in
`calculate_max_diving_workers`, `update_diving_heuristic_list`, or the caller
that launches diving workers so `max_diving_workers` is forced to 0 whenever no
diving heuristics are enabled, and ensure `launch_diving_worker` is never
reached in that state. Also add a defensive empty-list check in
`next_diving_heuristic()` to avoid UB if it is called unexpectedly.
In `@cpp/src/mip_heuristics/diversity/diversity_manager.cu`:
- Around line 207-210: The debug log in diversity_manager.cu is using the wrong
printf specifiers for size_t values, which can misprint or trigger undefined
behavior. Update the CUOPT_LOG_DEBUG call in the Crushed initial solution
message so the size_t arguments like sol_idx and h_crushed.size() use %zu
consistently, matching the earlier logging pattern in this flow. Keep the
existing variables and message structure, just correct the format string to
match the argument types.
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp`:
- Around line 1294-1298: The coeff_key lambda in third_party_presolve.cpp is
doing row-major key packing in 32-bit arithmetic, which can overflow and collide
in coeff_current for larger instances. Update the key computation and the
coeff_current key type used by crush_primal_dual_solution-related tracking to a
64-bit integer so row * n_cols_original + col cannot wrap, and adjust any
related lookups/inserts to use the widened key consistently.
In `@cpp/tests/dual_simplex/unit_tests/solve.cpp`:
- Around line 412-416: The call to convert_simplex_problem in solve.cpp passes
an extra dualize_info argument that does not match the function’s 5-parameter
signature. Update the recovery path around simplex::convert_simplex_problem and
user_problem_t<int, double> recovered so it passes only simplex_problem,
var_types, settings, new_slacks, and recovered, removing the unsupported
dualize_info argument.
In `@cpp/tests/mip/incumbent_callback_test.cu`:
- Around line 39-51: scoped_env_restore_t currently restores an originally unset
environment variable as an empty string instead of removing it, which leaks
global state across tests. Update scoped_env_restore_t to track whether the
variable existed before construction, and in ~scoped_env_restore_t use
unsetenv(name_) when it was originally absent; keep using setenv only when a
previous value was captured. Refer to scoped_env_restore_t, its constructor, and
destructor to make the fix.
In `@cpp/tests/mip/presolve_test.cu`:
- Around line 93-99: The presolve test is comparing the returned value from
presolver_t::apply() against the wrong enum type. Update the EXPECT_EQ in the
test that uses presolver.apply(user_problem, settings) to assert against
mip::mip_status_t::OPTIMAL instead of
mip::third_party_presolve_status_t::OPTIMAL, keeping the rest of the
empty-reduced-problem checks unchanged.
---
Outside diff comments:
In `@cpp/src/dual_simplex/simplex_solver_settings.hpp`:
- Around line 69-73: The default initialization for bnb_iteration_limit in
simplex_solver_settings should use the full 64-bit sentinel instead of i_t max,
since the member is int64_t and should match its declared width. Update the
constructor/initializer in simplex_solver_settings so bnb_iteration_limit is
initialized with the int64_t maximum value, keeping it consistent with the other
limit fields and avoiding an unintended 32-bit cap.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/pseudo_costs.hpp`:
- Around line 130-141: The copy assignment in pseudo_costs_t::operator= is
missing the warm_start state, so copied warm-started instances lose their
preserved pseudo-cost data before a later resize() call. Update
pseudo_costs_t::operator= to copy warm_start alongside AT, pdlp_warm_cache, and
the pseudo_cost_* fields, and make sure the same state is preserved consistently
in the copy-construction path that shares this assignment behavior.
In `@cpp/src/dual_simplex/presolve.cpp`:
- Around line 688-767: Add defensive checks in convert_simplex_problem to
enforce its implicit preconditions before rebuilding user_problem_t: verify that
new_slacks contains exactly one slack/artificial column per row and that the
simplex problem was not dualized. Use the existing symbols
simplex_problem.num_rows, new_slacks, and the convert_simplex_problem setup to
assert or fail fast when these assumptions are not met, so the row_sense/rhs
reconstruction cannot silently leave rows at the default equality state.
🪄 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: 02762e0c-8ec9-40b4-aa15-b48536aebd5f
📒 Files selected for processing (29)
cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hppcpp/src/barrier/barrier.cucpp/src/branch_and_bound/CMakeLists.txtcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/constants.hppcpp/src/branch_and_bound/deterministic_workers.hppcpp/src/branch_and_bound/diving_heuristics.hppcpp/src/branch_and_bound/mip_node.hppcpp/src/branch_and_bound/presolve.cppcpp/src/branch_and_bound/presolve.hppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/branch_and_bound/pseudo_costs.hppcpp/src/branch_and_bound/worker.hppcpp/src/cuts/cuts.cppcpp/src/dual_simplex/presolve.cppcpp/src/dual_simplex/presolve.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/diversity/lns/rins.cucpp/src/mip_heuristics/diversity/recombiners/sub_mip.cuhcpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/presolve/third_party_presolve.hppcpp/src/mip_heuristics/solve.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/tests/linear_programming/unit_tests/presolve_test.cucpp/tests/mip/incumbent_callback_test.cucpp/tests/mip/presolve_test.cudatasets/mip/download_miplib_test_dataset.sh
Comment on lines
+39
to
+51
| scoped_env_restore_t(const char* env_name, const char* new_value) : name_(env_name) | ||
| { | ||
| if (const char* prev = std::getenv(env_name)) { prev_value_ = prev; } | ||
| ::setenv(env_name, new_value, 1); | ||
| } | ||
| ~scoped_env_restore_t() { ::setenv(name_, prev_value_.c_str(), 1); } | ||
| scoped_env_restore_t(const scoped_env_restore_t&) = delete; | ||
| scoped_env_restore_t& operator=(const scoped_env_restore_t&) = delete; | ||
|
|
||
| private: | ||
| const char* name_; | ||
| std::string prev_value_; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
scoped_env_restore_t leaves the variable set to "" when it was originally unset.
The constructor only captures a previous value when one exists, but the destructor unconditionally calls setenv(name_, prev_value_.c_str(), 1). If the variable was not present before, it is restored as an empty string rather than removed, mutating global process state for subsequent tests. Track presence and unsetenv in that case.
As per path instructions: "Test isolation — no leaked GPU state or global mutation across tests".
🐛 Proposed fix
scoped_env_restore_t(const char* env_name, const char* new_value) : name_(env_name)
{
- if (const char* prev = std::getenv(env_name)) { prev_value_ = prev; }
+ if (const char* prev = std::getenv(env_name)) {
+ prev_value_ = prev;
+ had_value_ = true;
+ }
::setenv(env_name, new_value, 1);
}
- ~scoped_env_restore_t() { ::setenv(name_, prev_value_.c_str(), 1); }
+ ~scoped_env_restore_t()
+ {
+ if (had_value_) {
+ ::setenv(name_, prev_value_.c_str(), 1);
+ } else {
+ ::unsetenv(name_);
+ }
+ }
scoped_env_restore_t(const scoped_env_restore_t&) = delete;
scoped_env_restore_t& operator=(const scoped_env_restore_t&) = delete;
private:
const char* name_;
std::string prev_value_;
+ bool had_value_ = false;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| scoped_env_restore_t(const char* env_name, const char* new_value) : name_(env_name) | |
| { | |
| if (const char* prev = std::getenv(env_name)) { prev_value_ = prev; } | |
| ::setenv(env_name, new_value, 1); | |
| } | |
| ~scoped_env_restore_t() { ::setenv(name_, prev_value_.c_str(), 1); } | |
| scoped_env_restore_t(const scoped_env_restore_t&) = delete; | |
| scoped_env_restore_t& operator=(const scoped_env_restore_t&) = delete; | |
| private: | |
| const char* name_; | |
| std::string prev_value_; | |
| }; | |
| scoped_env_restore_t(const char* env_name, const char* new_value) : name_(env_name) | |
| { | |
| if (const char* prev = std::getenv(env_name)) { | |
| prev_value_ = prev; | |
| had_value_ = true; | |
| } | |
| ::setenv(env_name, new_value, 1); | |
| } | |
| ~scoped_env_restore_t() | |
| { | |
| if (had_value_) { | |
| ::setenv(name_, prev_value_.c_str(), 1); | |
| } else { | |
| ::unsetenv(name_); | |
| } | |
| } | |
| scoped_env_restore_t(const scoped_env_restore_t&) = delete; | |
| scoped_env_restore_t& operator=(const scoped_env_restore_t&) = delete; | |
| private: | |
| const char* name_; | |
| std::string prev_value_; | |
| bool had_value_ = false; | |
| }; |
🤖 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/tests/mip/incumbent_callback_test.cu` around lines 39 - 51,
scoped_env_restore_t currently restores an originally unset environment variable
as an empty string instead of removing it, which leaks global state across
tests. Update scoped_env_restore_t to track whether the variable existed before
construction, and in ~scoped_env_restore_t use unsetenv(name_) when it was
originally absent; keep using setenv only when a previous value was captured.
Refer to scoped_env_restore_t, its constructor, and destructor to make the fix.
Source: Path instructions
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…sible. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…mber of iterations when creating the RINS neighbourhood. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Contributor
Author
|
/ok to test d5f020e |
CI Test Summary3 failed · 10 passed · 2 skipped
|
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
2 participants
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 implements the recursive subMIP from HiGHS.
Issue
Checklist