Papilo primal/dual crush#1104
Conversation
aliceb-nv
added
non-breaking
|
/ok to test d47d709 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughAdds forward "crush" mappings to transform original-space solutions into Papilo reduced space, integrates crushing into initial-solution workflows (including GPU-disabled path), adjusts Papilo presolve selection and column-integrality handling, introduces RAII guards for OpenMP resource cleanup, and collects multiple early incumbents for improved population seeding. ChangesPapilo Crushing and Solution-Space Mapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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: 4
🤖 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/mip_heuristics/presolve/third_party_presolve.cpp`:
- Around line 1092-1096: The loop that updates z for removed rows uses an exact
check y[i] == 0 which is too strict; replace that check with the
function/epsilon-based test used elsewhere in this function (i.e., treat y as
zero when fabs(y[i]) <= the existing numeric tolerance or by calling the
existing isZero/is_near_zero helper) so that only truly near-zero y[i] are
skipped; locate the loop referencing storage.nRowsOriginal, row_survives, y,
A_offsets, A_indices, z and get_coeff and change the condition to use the same
tolerance variable or helper used elsewhere in this file to avoid injecting
noise into z (and therefore z_reduced).
- Around line 973-980: The kParallelCol case only updates the primal x but fails
to fold reduced costs; update the corresponding dual/reduced-cost vector (z)
analogously so eliminated parallel-column contributions aren't lost: inside the
ReductionType::kParallelCol branch (where col1 = indices[first], col2 =
indices[first+2], scale = values[first+4] and x[col2] += scale * x[col1]) also
perform z[col2] += scale * z[col1] (using the same index mapping and scale),
ensuring you access the z array from the same problem context and respect any
index-mapping helpers used elsewhere in this function.
In `@cpp/tests/linear_programming/unit_tests/presolve_test.cu`:
- Around line 859-861: The test currently uses EXPECT_LT(warm_iters, cold_iters)
which enforces a strict decrease and makes the test flaky; change the assertion
to EXPECT_LE(warm_iters, cold_iters) so the warm-started PDLP is allowed to take
the same number of iterations as the cold run. Update the failure message if
desired but keep the same variables (warm_iters, cold_iters) and replace the
EXPECT_LT macro with EXPECT_LE in the presolve_test assertion.
In `@cpp/tests/mip/incumbent_callback_test.cu`:
- Around line 41-44: The destructor of scoped_env_restore_t always calls
::setenv(name_, prev_value_.c_str(), 1) which re-creates the variable as an
empty string if it was originally unset; change scoped_env_restore_t to record
whether the environment var existed (e.g. a bool prev_exists_ set in the
constructor when std::getenv(env_name) != nullptr) and in
~scoped_env_restore_t() call ::unsetenv(name_) when prev_exists_ is false,
otherwise restore the original value via ::setenv using prev_value_; update the
constructor and member fields (prev_exists_ and prev_value_) accordingly and
ensure behavior is consistent for CUOPT_DISABLE_GPU_HEURISTICS and similar uses.
🪄 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 Plus
Run ID: 81564330-0c16-431c-9cbc-81de259afadb
📒 Files selected for processing (7)
cpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/presolve/third_party_presolve.hppcpp/src/mip_heuristics/solve.cucpp/tests/linear_programming/unit_tests/presolve_test.cucpp/tests/mip/incumbent_callback_test.cudatasets/mip/download_miplib_test_dataset.sh
| case ReductionType::kParallelCol: { | ||
| // Storage layout: [orig_col1, flags1, orig_col2, flags2, -1] | ||
| // [col1lb, col1ub, col2lb, col2ub, col2scale] | ||
| int col1 = indices[first]; | ||
| int col2 = indices[first + 2]; | ||
| const f_t& scale = values[first + 4]; | ||
| x[col2] += scale * x[col1]; | ||
| break; |
There was a problem hiding this comment.
Fold reduced costs through kParallelCol as well.
This forward replay updates the survivor primal value, but it leaves z in the original basis. After the final projection, any eliminated parallel column contribution is dropped, so z_reduced is wrong whenever dual/reduced-cost crushing hits a parallel-column reduction.
🐛 Suggested fix
case ReductionType::kParallelCol: {
// Storage layout: [orig_col1, flags1, orig_col2, flags2, -1]
// [col1lb, col1ub, col2lb, col2ub, col2scale]
int col1 = indices[first];
int col2 = indices[first + 2];
const f_t& scale = values[first + 4];
x[col2] += scale * x[col1];
+ if (crush_rc) { z[col2] += scale * z[col1]; }
break;
}As per coding guidelines, **/*.{cu,cuh,cpp,hpp,h}: Validate algorithm correctness in optimization logic and ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp` around lines 973 -
980, The kParallelCol case only updates the primal x but fails to fold reduced
costs; update the corresponding dual/reduced-cost vector (z) analogously so
eliminated parallel-column contributions aren't lost: inside the
ReductionType::kParallelCol branch (where col1 = indices[first], col2 =
indices[first+2], scale = values[first+4] and x[col2] += scale * x[col1]) also
perform z[col2] += scale * z[col1] (using the same index mapping and scale),
ensuring you access the z array from the same problem context and respect any
index-mapping helpers used elsewhere in this function.
| for (int i = 0; i < (int)storage.nRowsOriginal; ++i) { | ||
| if (row_survives[i] || y[i] == 0) continue; | ||
| for (i_t p = A_offsets[i]; p < A_offsets[i + 1]; ++p) { | ||
| z[A_indices[p]] += y[i] * get_coeff(i, A_indices[p]); | ||
| } |
There was a problem hiding this comment.
Avoid exact-zero checks on removed-row duals.
y[i] == 0 is too strict for approximate PDLP/DualSimplex duals. Near-zero removed-row multipliers will still enter this correction path and inject noise into z_reduced. Use the same numeric tolerance machinery you already use elsewhere in this function.
As per coding guidelines, **/*.{cu,cuh,cpp,hpp,h}: Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp` around lines 1092 -
1096, The loop that updates z for removed rows uses an exact check y[i] == 0
which is too strict; replace that check with the function/epsilon-based test
used elsewhere in this function (i.e., treat y as zero when fabs(y[i]) <= the
existing numeric tolerance or by calling the existing isZero/is_near_zero
helper) so that only truly near-zero y[i] are skipped; locate the loop
referencing storage.nRowsOriginal, row_survives, y, A_offsets, A_indices, z and
get_coeff and change the condition to use the same tolerance variable or helper
used elsewhere in this file to avoid injecting noise into z (and therefore
z_reduced).
| EXPECT_LT(warm_iters, cold_iters) | ||
| << "warmstarted solve should not take more iterations than cold solve" | ||
| << " (cold=" << cold_iters << ", warm=" << warm_iters << ")"; |
There was a problem hiding this comment.
Don’t require a strict iteration drop here.
Warm-started PDLP can legitimately converge in the same number of iterations as the cold run because of fixed startup/restart behavior. EXPECT_LT makes this test flaky even when the warm start is working.
💡 Suggested fix
- EXPECT_LT(warm_iters, cold_iters)
+ EXPECT_LE(warm_iters, cold_iters)
<< "warmstarted solve should not take more iterations than cold solve"
<< " (cold=" << cold_iters << ", warm=" << warm_iters << ")";📝 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.
| EXPECT_LT(warm_iters, cold_iters) | |
| << "warmstarted solve should not take more iterations than cold solve" | |
| << " (cold=" << cold_iters << ", warm=" << warm_iters << ")"; | |
| EXPECT_LE(warm_iters, cold_iters) | |
| << "warmstarted solve should not take more iterations than cold solve" | |
| << " (cold=" << cold_iters << ", warm=" << warm_iters << ")"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/tests/linear_programming/unit_tests/presolve_test.cu` around lines 859 -
861, The test currently uses EXPECT_LT(warm_iters, cold_iters) which enforces a
strict decrease and makes the test flaky; change the assertion to
EXPECT_LE(warm_iters, cold_iters) so the warm-started PDLP is allowed to take
the same number of iterations as the cold run. Update the failure message if
desired but keep the same variables (warm_iters, cold_iters) and replace the
EXPECT_LT macro with EXPECT_LE in the presolve_test assertion.
| 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); } |
There was a problem hiding this comment.
Restore the original unset state, not an empty string.
If CUOPT_DISABLE_GPU_HEURISTICS was originally unset, the destructor currently leaves it defined as "". That leaks process-global state across tests and can change behavior for code that branches on std::getenv(...) != nullptr.
💡 Suggested fix
class scoped_env_restore_t {
public:
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)) {
+ had_prev_value_ = true;
+ prev_value_ = prev;
+ }
::setenv(env_name, new_value, 1);
}
- ~scoped_env_restore_t() { ::setenv(name_, prev_value_.c_str(), 1); }
+ ~scoped_env_restore_t()
+ {
+ if (had_prev_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_;
+ bool had_prev_value_ = false;
std::string prev_value_;
};As per coding guidelines, **/*test*.{cpp,cu,py}: Ensure test isolation: prevent GPU state, cached memory, and global variables from leaking between test cases; verify each test independently initializes its environment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/tests/mip/incumbent_callback_test.cu` around lines 41 - 44, The
destructor of scoped_env_restore_t always calls ::setenv(name_,
prev_value_.c_str(), 1) which re-creates the variable as an empty string if it
was originally unset; change scoped_env_restore_t to record whether the
environment var existed (e.g. a bool prev_exists_ set in the constructor when
std::getenv(env_name) != nullptr) and in ~scoped_env_restore_t() call
::unsetenv(name_) when prev_exists_ is false, otherwise restore the original
value via ::setenv using prev_value_; update the constructor and member fields
(prev_exists_ and prev_value_) accordingly and ensure behavior is consistent for
CUOPT_DISABLE_GPU_HEURISTICS and similar uses.
| * kRowBoundChangeForcedByRow — conditionally transfers y[deleted_row] → y[kept_row] | ||
| */ | ||
| template <typename i_t, typename f_t> | ||
| void third_party_presolve_t<i_t, f_t>::crush_primal_dual_solution( |
There was a problem hiding this comment.
Can we push this to Papilo itself?
When new presolvers (or reductions) are added, this can get outdated.
There was a problem hiding this comment.
That's on my radar :) There are a couple of PRs I will be pushing to Papilo
|
🔔 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. |
|
/ok to test 157045b |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/mip_heuristics/diversity/diversity_manager.cu`:
- Around line 439-441: The early return when check_b_b_preemption() is true can
skip loading warm-starts, causing initial_sol_vector not to be added to
population; move or duplicate the calls to
add_user_given_solutions(initial_sol_vector) and
population.add_solutions_from_vec(std::move(initial_sol_vector)) so they run
before the preemption check (or ensure they run unconditionally irrespective of
preempt_heuristic_solver_), keeping the check_b_b_preemption() return behavior
and references to population.best_feasible() unchanged.
- Around line 193-199: The code currently calls cuopt_assert when
init_sol_assignment.size() != papilo_orig_n inside the has_papilo branch;
instead, guard that condition with a runtime check and treat it as a recoverable
bad warm-start: if sizes differ, log/reject the warm-start entry and skip the
Papilo-specific path (do not call
problem_ptr->presolve_data.papilo_presolve_ptr->crush_primal_solution or
device_copy), allowing the existing “cannot add” handling to run later; replace
the cuopt_assert with an if that records the mismatch (using the same logging
mechanism used elsewhere) and continues without aborting so malformed user input
is handled gracefully.
🪄 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: 4f3f0adf-41f2-4749-89ad-2df9c433d3c6
📒 Files selected for processing (2)
cpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/solve.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/mip_heuristics/solve.cu
|
/ok to test 6f5f716 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mip_heuristics/diversity/diversity_manager.cu`:
- Line 480: The early return when check_b_b_preemption() is true can skip adding
user warm-starts; ensure initial_sol_vector is always moved into population
before any preemption return by calling
population.add_solutions_from_vec(std::move(initial_sol_vector)) prior to the
check_b_b_preemption() short-circuit, or alternatively defer the check and
replace the early return with a conditional that first adds the solutions then
returns population.best_feasible(); update the logic around
check_b_b_preemption(),
population.add_solutions_from_vec(std::move(initial_sol_vector)), and the
population.best_feasible() return to guarantee user-provided initial solutions
are added before exiting.
🪄 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: 0ba11407-611e-4f51-af1e-1f8fc3a89a98
📒 Files selected for processing (1)
cpp/src/mip_heuristics/diversity/diversity_manager.cu
| population.allocate_solutions(); | ||
| if (check_b_b_preemption()) { return population.best_feasible(); } | ||
| add_user_given_solutions(initial_sol_vector); | ||
| if (check_b_b_preemption()) { return population.best_feasible(); } |
There was a problem hiding this comment.
Early preemption can still drop user warm-starts in the main path.
At Line 480, check_b_b_preemption() can return before population.add_solutions_from_vec(std::move(initial_sol_vector)) (Line 616), so user-provided initial solutions may never reach population on early exit.
Suggested minimal fix
add_user_given_solutions(initial_sol_vector);
- if (check_b_b_preemption()) { return population.best_feasible(); }
+ if (check_b_b_preemption()) {
+ population.add_solutions_from_vec(std::move(initial_sol_vector));
+ return population.best_feasible();
+ }📝 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.
| if (check_b_b_preemption()) { return population.best_feasible(); } | |
| add_user_given_solutions(initial_sol_vector); | |
| if (check_b_b_preemption()) { | |
| population.add_solutions_from_vec(std::move(initial_sol_vector)); | |
| return population.best_feasible(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/diversity/diversity_manager.cu` at line 480, The early
return when check_b_b_preemption() is true can skip adding user warm-starts;
ensure initial_sol_vector is always moved into population before any preemption
return by calling
population.add_solutions_from_vec(std::move(initial_sol_vector)) prior to the
check_b_b_preemption() short-circuit, or alternatively defer the check and
replace the early return with a conditional that first adds the solutions then
returns population.best_feasible(); update the logic around
check_b_b_preemption(),
population.add_solutions_from_vec(std::move(initial_sol_vector)), and the
population.best_feasible() return to guarantee user-provided initial solutions
are added before exiting.
|
🔔 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. |
2 similar comments
|
🔔 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. |
|
🔔 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. |
|
Moving to 26.08 |
|
🔔 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. |
3 similar comments
|
🔔 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. |
|
🔔 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. |
|
🔔 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. |
|
/ok to test a6a6a1b |
This reverts commit 3025d8e.
|
/ok to test 0174bee |
| presolver.addPresolveMethod(uptr(new papilo::Probing<f_t>())); | ||
|
|
||
| if (!dual_postsolve) { | ||
| // SingletonStuffing causes dual crushing failures on: |
There was a problem hiding this comment.
There is no dual crush for MIP right?
|
|
||
| // Track current coefficient values for entries modified by kCoefficientChange, | ||
| // so repeated changes to the same (row, col) are handled correctly. | ||
| std::map<std::pair<int, int>, f_t> coeff_current; |
There was a problem hiding this comment.
This can be very slow.
Can you avoid map? Can you just store a CSR/COO of the size of the original matrix?
4 participants
This PR implements support for crushing primal incumbents in MIP mode into the Papilo problem space, and crushing primal/dual vectors for LP.
A bugfix is also included to allow consecutive solves to be run in the same GTest process without corrupting the OpenMP runtime.
Closes #513
Closes #1060
Description
Issue
Checklist