Integrate PSLP presolver in cuOpt#816
Conversation
rg20
added
feature request
📝 WalkthroughWalkthroughReplaces boolean presolve flags with a new presolver_t enum across settings, solvers, tests and docs; integrates PSLP as a static FetchContent dependency and exposes its headers; adds PSLP presolve plumbing and cleanup; refactors folding to ordered maps and status returns; fixes histogram/dense-row logic; adds LP solver overloads. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/tests/CMakeLists.txt (1)
40-46: Remove the hard-coded PSLP include path.The absolute
/home/nfs/...path will break builds on any other machine/CI. Please use the PSLP target’s include dirs or FetchContent variables.🔧 Suggested fix (target-based include)
- "/home/nfs/rgandham/git-repos/PSLP/include/PSLP" + "$<TARGET_PROPERTY:PSLP,INTERFACE_INCLUDE_DIRECTORIES>"cpp/src/mip/presolve/third_party_presolve.cpp (1)
629-691: Add null check and postsolve error handling inundo_pslp.The
undo_pslpmethod dereferencespslp_presolver_without validation (lines 682, 689–691) and doesn't check thepostsolve()result. In contrast, the Papilo postsolve path explicitly checks status viacheck_postsolve_status()(line 655). Add a guard assertion and status check to match the error handling pattern used for Papilo:🧯 Guard against uninitialized PSLP state
void third_party_presolve_t<i_t, f_t>::undo_pslp(rmm::device_uvector<f_t>& primal_solution, rmm::device_uvector<f_t>& dual_solution, rmm::device_uvector<f_t>& reduced_costs, rmm::cuda_stream_view stream_view) { + cuopt_expects(pslp_presolver_ != nullptr, + error_type_t::RuntimeError, + "PSLP presolver not initialized"); std::vector<f_t> h_primal_solution(primal_solution.size());
🤖 Fix all issues with AI agents
In `@cpp/src/dual_simplex/barrier.cu`:
- Around line 1155-1158: The histogram output loop omits the last bin (nnz == n)
because histogram_row has size n+1 but the for loop uses "for (i_t k = 0; k < n;
k++)"; update the loop that prints histogram_row (the block using
settings.log.printf and the loop variable k) to iterate through all n+1 bins
(e.g., change the loop condition to include k == n) so entries where row_nz == n
are printed.
In `@cpp/src/math_optimization/solver_settings.cu`:
- Around line 91-93: The CUOPT_PRESOLVE parameter is now treated as an int but
callers (e.g., set_parameter_from_string or boolean setters) may still pass
"true"/"false" or bools; update the parameter parsing/shim to accept
boolean-like values and map them to the int enums (e.g., DEFAULT/OFF) before
assigning to pdlp_settings.presolver and mip_settings.presolver, or add a branch
in the string-to-parameter logic that recognizes "true"/"false"/"1"/"0" and
converts them to the corresponding CUOPT_PRESOLVE_* constants so existing
boolean-based calls do not throw when setting CUOPT_PRESOLVE.
In `@cpp/src/mip/presolve/third_party_presolve.cpp`:
- Around line 528-539: Before overwriting pslp_presolver_ and pslp_stgs_ in
third_party_presolve_t<i_t,f_t>::apply_pslp, check if they are non-null and
release their prior allocations (call the appropriate PSLP destructor/free
function or delete depending on how PSLP objects are managed), then set them to
the new ctx.presolver and ctx.settings; specifically, free the existing
pslp_presolver_ and pslp_stgs_ members before assigning new values to avoid
leaking prior PSLP resources when apply_pslp is called repeatedly.
- Line 8: Remove the unused include of boost/serialization/void_cast.hpp from
third_party_presolve.cpp: delete the line "#include
<boost/serialization/void_cast.hpp>" (ensure no other code in this file
references Boost Serialization or void_cast so the build remains clean).
- Around line 180-283: Replace the assert and ignored presolve result in
build_and_run_pslp_presolver with explicit runtime checks: change the function
to return std::optional<PSLPContext>, after calling new_presolver(...) check if
ctx.presolver is nullptr and return std::nullopt (cleaning up settings if
needed); then call run_presolver(ctx.presolver) and if the returned
PresolveStatus is not success (explicitly handle kInfeasible, kUnbndOrInfeas and
any non-success), free/delete ctx.presolver and return std::nullopt; only return
a populated PSLPContext when new_presolver succeeded and run_presolver returned
success, ensuring callers (e.g., code reading ctx.presolver->reduced_prob) won't
dereference an invalid presolver.
In `@cpp/src/mip/presolve/third_party_presolve.hpp`:
- Around line 58-76: The class currently owning raw pointers pslp_stgs_ and
pslp_presolver_ can be accidentally copied and cause double-free; update the
third_party_presolve_t class to be non-copyable/non-movable by explicitly
deleting the copy constructor, copy assignment operator, move constructor, and
move assignment operator (i.e., add declarations like
third_party_presolve_t(const third_party_presolve_t&) = delete; and similar for
operator= and move variants) so instances that own pslp_stgs_ and
pslp_presolver_ cannot be copied or moved; alternatively, replace those raw
pointers with RAII types (unique_ptr) if you prefer move-only semantics, but the
quick fix is to delete copy/move for third_party_presolve_t.
In `@cpp/src/mip/solve.cu`:
- Around line 150-153: The code in the mip_solver_settings_t constructor block
currently silently replaces settings.presolver when it's presolver_t::Default or
presolver_t::PSLP with presolver_t::Papilo; update this to either emit a clear
warning or return an error instead of silently overriding: detect when
settings.presolver == presolver_t::PSLP (and optionally when ==
presolver_t::Default), log a warning via the existing logging mechanism (or
propagate an error/exception) that "PSLP not supported for MIP, falling back to
Papilo" (or require user change), and only perform the assignment to
presolver_t::Papilo after logging/handling so the change is explicit; refer to
mip_solver_settings_t, settings.presolver and presolver_t::PSLP /
presolver_t::Papilo when locating and modifying the code.
🧹 Nitpick comments (2)
benchmarks/linear_programming/run_mps_files.sh (1)
417-417: Make tolerances overrideable (and verify CLI flag support).
Hard-coding 1.0e-6 across all runs limits benchmarking flexibility; consider env-based defaults so users can adjust without editing the script. Also please confirm these flags are supported by the cuopt_cli version used in CI to avoid runtime failures.♻️ Example with env-defaults (single-line change)
- CUDA_VISIBLE_DEVICES=$gpu_devices cuopt_cli "$mps_file" --time-limit $TIME_LIMIT $args --relative-primal-tolerance 1.0e-6 --relative-dual-tolerance 1.0e-6 --relative-gap-tolerance 1.0e-6 --absolute-primal-tolerance 1.0e-6 --absolute-dual-tolerance 1.0e-6 --absolute-gap-tolerance 1.0e-6 + CUDA_VISIBLE_DEVICES=$gpu_devices cuopt_cli "$mps_file" --time-limit $TIME_LIMIT $args \ + --relative-primal-tolerance ${RELATIVE_PRIMAL_TOLERANCE:-1.0e-6} \ + --relative-dual-tolerance ${RELATIVE_DUAL_TOLERANCE:-1.0e-6} \ + --relative-gap-tolerance ${RELATIVE_GAP_TOLERANCE:-1.0e-6} \ + --absolute-primal-tolerance ${ABSOLUTE_PRIMAL_TOLERANCE:-1.0e-6} \ + --absolute-dual-tolerance ${ABSOLUTE_DUAL_TOLERANCE:-1.0e-6} \ + --absolute-gap-tolerance ${ABSOLUTE_GAP_TOLERANCE:-1.0e-6}cpp/src/dual_simplex/folding.cpp (1)
224-280: Guard non‑finite sums before using them asstd::mapkeys.Line 247 and Line 279 insert floating sums into ordered maps; if any sum is NaN/Inf,
std::map’s ordering is violated and keys can collapse, leading to incorrect splits. Consider a lightweight finiteness check before insertion.🛠️ Suggested guard
sum_to_sizes.clear(); color_sums.clear(); for (i_t v : vertices_to_refine_by_color[color]) { - sum_to_sizes[vertex_to_sum[v]]++; + const f_t sum = vertex_to_sum[v]; + if (!std::isfinite(sum)) { + printf("Folding: non-finite sum for vertex %d\n", v); + exit(1); + } + sum_to_sizes[sum]++; }As per coding guidelines: Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/tests/linear_programming/pdlp_test.cu (1)
1615-1615: Inconsistent API usage:presolveboolean instead ofpresolverenum.This line still uses the old
settings.presolve = falsepattern while the rest of the codebase has migrated tosettings.presolver = presolver_t::None. This appears to be a missed migration.🛠️ Proposed fix
- solver_settings.presolve = false; + solver_settings.presolver = presolver_t::None;
🤖 Fix all issues with AI agents
In `@cpp/src/mip/presolve/third_party_presolve.cpp`:
- Around line 682-691: undo_pslp() is resizing and copying solution outputs
using the reduced problem dimensions (pslp_presolver_->reduced_prob->m/n) which
truncates postsolved outputs; instead, capture the original problem dimensions
in apply_pslp() (e.g., store original m and n on the pslp_presolver_ object or a
dedicated members like orig_m/orig_n when you create reduced_prob) and then use
those stored original dimensions in undo_pslp() when resizing primal_solution,
dual_solution, reduced_costs and when copying from pslp_presolver_->sol->x/y/z
so the postsolve-expanded solutions fit the original problem size.
In `@cpp/tests/CMakeLists.txt`:
- Line 45: Remove the hard-coded absolute include path
"/home/nfs/rgandham/git-repos/PSLP/include/PSLP" from the
target_include_directories call in cpp/tests/CMakeLists.txt; the test target
already links to cuopt (which has PSLP as a PRIVATE dependency via
FetchContent), so simply delete that literal entry from
target_include_directories to rely on transitive includes from the cuopt target.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/linear_programming/solve.cu (1)
1146-1162: Presolve time log is now misleading for PSLP.
The message still says “Papilo presolve time” even when PSLP is selected, which makes logs confusing.🔧 Proposed change
- CUOPT_LOG_INFO("Papilo presolve time: %f", presolve_time); + CUOPT_LOG_INFO("Presolve time: %f", presolve_time);
🧹 Nitpick comments (1)
cpp/tests/linear_programming/pdlp_test.cu (1)
214-237: Add PSLP (and optionally Default) coverage in the presolver loop.This test is the most visible place to exercise the new presolver options; currently only Papilo/None are covered. Adding PSLP here would validate the new integration path.
🔧 Suggested test expansion
- for (auto [presolver, epsilon] : - {std::pair{presolver_t::Papilo, 1e-1}, std::pair{presolver_t::None, 1e-6}}) { + for (auto [presolver, epsilon] : + {std::pair{presolver_t::Papilo, 1e-1}, + std::pair{presolver_t::PSLP, 1e-1}, + std::pair{presolver_t::None, 1e-6}}) {
|
I have a question: the PSLP will be only applicable for LP or it will be also be used for MIP as well? |
cpp/src/dual_simplex/folding.cpp
Outdated
| if (!largest_is_sum_zero && color_sums.count(0.0) > 0) { | ||
| // sum=0 exists but is NOT the largest - need to create a new color for it | ||
| // These are vertices in the color that don't touch the refining color | ||
| std::unordered_set<i_t> refined_set(vertices_to_refine_by_color[color].begin(), |
There was a problem hiding this comment.
We don't need to create a refined_set to know if a vertex was not touched by the refining color.
cpp/src/dual_simplex/folding.cpp
Outdated
| std::unordered_set<i_t> refined_set(vertices_to_refine_by_color[color].begin(), | ||
| vertices_to_refine_by_color[color].end()); | ||
| for (i_t v : colors[color].vertices) { | ||
| if (refined_set.find(v) == refined_set.end()) { sum_zero_vertices.push_back(v); } |
There was a problem hiding this comment.
I think this condition should be:
if (vertex_to_sum[v] == 0.0) { sum_zero_vertices.push_back(v); }
There was a problem hiding this comment.
Actually, we don't need to have a separate sum_zero_vertices array at all. We should just append these to color_sums
if (vertex_to_sum[v] == 0.0) { color_sums[0.0].push_back(v) }
|
/ok to test dceac9a |
|
/ok to test 528ddea |
done |
There was a problem hiding this comment.
Actionable comments posted: 1
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/folding.cpp (1)
251-266:⚠️ Potential issue | 🟡 MinorUnsigned subtraction of
size_tvalues may not be caught by the< 0check.
colors[color].vertices.size()andvertices_to_refine_by_color[color].size()both returnsize_t. The subtraction is performed in unsigned arithmetic, so if the second operand is larger, the result wraps to a huge positive value. Assigning this toi_t(signedint) is implementation-defined and theremaining_size < 0check may not reliably detect the error condition.Proposed fix: cast before subtraction
- const i_t remaining_size = - colors[color].vertices.size() - vertices_to_refine_by_color[color].size(); + const i_t remaining_size = + static_cast<i_t>(colors[color].vertices.size()) - static_cast<i_t>(vertices_to_refine_by_color[color].size());As per coding guidelines: "Validate algorithm correctness in optimization logic... constraint/objective handling must produce correct results."
🤖 Fix all issues with AI agents
In `@cpp/src/dual_simplex/folding.cpp`:
- Around line 711-712: The log call using settings.log.printf in folding.cpp
(the line that prints "Folding: %d refinements %d colors in %.2fs") is missing a
trailing newline; update that settings.log.printf invocation to include a "\n"
at the end of the format string so the message ends with a newline (keep the
same format specifiers and arguments: num_refinements, current_colors, elapsed).
🧹 Nitpick comments (1)
cpp/src/dual_simplex/folding.cpp (1)
1440-1451:operator[]onunordered_mapsilently inserts default entries during verification.Lines 1441 and 1447–1448 use
operator[]onrow_col_color_sum, which inserts a default0.0entry if the key doesn't exist. This is functionally correct (missing entries represent zero sums), but it mutates the lookup maps during what is conceptually a read-only check, bloating memory. The column-side check at line 1481 correctly uses.find()instead. Since this is DEBUG-only code, this is low priority.Use `.find()` or `.count()` consistently
for (auto& [cc, ref_sum] : row_col_color_sum[ref_row]) { - f_t u_sum = row_col_color_sum[u][cc]; + f_t u_sum = 0.0; + auto it_u = row_col_color_sum[u].find(cc); + if (it_u != row_col_color_sum[u].end()) { u_sum = it_u->second; } f_t diff = std::abs(ref_sum - u_sum);Similarly for line 1447–1448:
for (auto& [cc, u_sum] : row_col_color_sum[u]) { - f_t ref_sum = row_col_color_sum[ref_row][cc]; + f_t ref_sum = 0.0; + auto it_r = row_col_color_sum[ref_row].find(cc); + if (it_r != row_col_color_sum[ref_row].end()) { ref_sum = it_r->second; } f_t diff = std::abs(ref_sum - u_sum);
|
/ok to test 78f2877 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/src/dual_simplex/folding.cpp (2)
252-265:⚠️ Potential issue | 🟡 MinorUnsigned subtraction may silently wrap instead of going negative.
Both
.size()calls returnsize_t(unsigned). Ifvertices_to_refine_by_color[color].size()exceedscolors[color].vertices.size(), the subtraction wraps to a very large unsigned value before assignment toi_t. On most two's-complement systems this converts to negative, but it's implementation-defined and could silently yield a positivei_ton some platforms, bypassing the< 0guard.Proposed fix
- const i_t remaining_size = - colors[color].vertices.size() - vertices_to_refine_by_color[color].size(); - if (remaining_size < 0) { + if (vertices_to_refine_by_color[color].size() > colors[color].vertices.size()) { printf("Negative remaining size %d\n", remaining_size); ... + return -1; + } + const i_t remaining_size = + static_cast<i_t>(colors[color].vertices.size() - vertices_to_refine_by_color[color].size()); + {The same pattern appears at lines 314-315. As per coding guidelines, validate correct initialization and bounds before operations.
738-741:⚠️ Potential issue | 🟡 MinorLog message missing trailing newline.
Line 739 is missing
\n, which will concatenate with the next log line. This is the same class of issue that was fixed at line 712.Proposed fix
- settings.log.printf("Folding: Number of colors exceeds threshold"); + settings.log.printf("Folding: Number of colors exceeds threshold\n");
🧹 Nitpick comments (1)
cpp/src/dual_simplex/folding.cpp (1)
404-405:color_lower_boundsstill usesstd::unordered_map<f_t, ...>with floating-point keys.The PR specifically switches
color_sumsandsum_to_sizesfromunordered_maptomapto avoid floating-point hashing issues, butunique_row_sumsandunique_col_sumshere use the same problematic pattern. This function is called once (not on the hot path), so it's a consistency/correctness concern rather than performance.Suggested change for consistency
- std::unordered_map<f_t, i_t> unique_row_sums; - std::unordered_map<f_t, i_t> unique_col_sums; + std::map<f_t, i_t> unique_row_sums; + std::map<f_t, i_t> unique_col_sums;
|
/ok to test 3835de5 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cpp/tests/linear_programming/unit_tests/presolve_test.cu`:
- Around line 381-384: The relative-error computation uses expected_obj as
divisor which can be zero; update the check around where primal_obj,
expected_obj and rel_error are computed (using
solution.get_additional_termination_information()) to guard against
division-by-zero: if expected_obj is effectively zero (abs(expected_obj) <
small_eps) compute an absolute error (abs(primal_obj - expected_obj)) and assert
with EXPECT_LT/EXPECT_NEAR accordingly, otherwise compute the relative error as
before and use EXPECT_LT; keep the existing failure message (<< "Problem " <<
name << " objective mismatch") and apply this branch to the
rel_error/expectation logic.
🧹 Nitpick comments (4)
cpp/tests/linear_programming/pdlp_test.cu (1)
214-238:run_sub_mittlemandoesn't exercise the new PSLP presolver.The loop tests
presolver_t::Papiloandpresolver_t::Nonebut omitspresolver_t::PSLP, which is the primary feature of this PR. While dedicated PSLP tests exist inpresolve_test.cu, the Mittelman suite provides broader instance coverage. Consider addingpresolver_t::PSLPto the presolver loop here, potentially with a relaxed epsilon similar to Papilo.cpp/tests/linear_programming/unit_tests/presolve_test.cu (3)
166-199: Weak duality check uses a very loose tolerance.
EXPECT_NEAR(primal_obj, dual_obj, 1.0)allows up to 1.0 absolute gap for an objective around -464. Consider tightening this or using a relative tolerance, and/or verifying a few dual variable values rather than just the vector size.
322-348: Reduced costs test only validates vector size, not values.This test verifies that the postsolved reduced costs vector has the right dimension but doesn't check any numerical properties (e.g., reduced costs should be non-negative for non-basic variables at their lower bounds in a minimization LP). Even a basic sanity check would strengthen coverage. As per coding guidelines, tests should validate numerical correctness of optimization results, not just "runs without error."
351-386: Consider extending the multi-problem test to cover more instances and edge cases.Currently only
afiro_originalandex10are tested with PSLP. Consider adding a few more instances from the Mittelman suite (e.g.,datt256_lp,savsched1) and at least one degenerate case (empty or trivially infeasible problem) to validate PSLP postsolve robustness. As per coding guidelines, tests should cover degenerate cases such as infeasible, unbounded, empty, and singleton problems.
|
|
||
| double primal_obj = solution.get_additional_termination_information().primal_objective; | ||
| double rel_error = std::abs((primal_obj - expected_obj) / expected_obj); | ||
| EXPECT_LT(rel_error, 0.01) << "Problem " << name << " objective mismatch"; |
There was a problem hiding this comment.
Potential division by zero in relative error computation.
Line 383 computes std::abs((primal_obj - expected_obj) / expected_obj). If a test instance with expected_obj == 0.0 is added (e.g., woodlands09 from the Mittelman suite has objective 0.0), this produces undefined behavior. Guard against this case.
🛡️ Proposed fix
double primal_obj = solution.get_additional_termination_information().primal_objective;
- double rel_error = std::abs((primal_obj - expected_obj) / expected_obj);
- EXPECT_LT(rel_error, 0.01) << "Problem " << name << " objective mismatch";
+ if (expected_obj != 0.0) {
+ double rel_error = std::abs((primal_obj - expected_obj) / expected_obj);
+ EXPECT_LT(rel_error, 0.01) << "Problem " << name << " objective mismatch";
+ } else {
+ EXPECT_NEAR(primal_obj, 0.0, 1e-4) << "Problem " << name << " objective mismatch";
+ }🤖 Prompt for AI Agents
In `@cpp/tests/linear_programming/unit_tests/presolve_test.cu` around lines 381 -
384, The relative-error computation uses expected_obj as divisor which can be
zero; update the check around where primal_obj, expected_obj and rel_error are
computed (using solution.get_additional_termination_information()) to guard
against division-by-zero: if expected_obj is effectively zero (abs(expected_obj)
< small_eps) compute an absolute error (abs(primal_obj - expected_obj)) and
assert with EXPECT_LT/EXPECT_NEAR accordingly, otherwise compute the relative
error as before and use EXPECT_LT; keep the existing failure message (<<
"Problem " << name << " objective mismatch") and apply this branch to the
rel_error/expectation logic.
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
LGTM. Thanks for hooking up the PSLP presolver.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mip/presolve/third_party_presolve.cpp (1)
75-81:⚠️ Potential issue | 🟠 MajorMissing stream synchronization before accessing host vectors.
The
raft::copycalls (lines 55–75) are asynchronous onstream_view, but host vectors are read immediately at line 77. This is the same race condition that was correctly fixed inbuild_and_run_pslp_presolver(line 238). Add synchronization here for consistency and correctness.Proposed fix
raft::copy(h_var_types.data(), var_types.data(), var_types.size(), stream_view); + stream_view.synchronize(); + if (maximize) { for (size_t i = 0; i < h_obj_coeffs.size(); ++i) {As per coding guidelines: "Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution" — but synchronization is required here before host reads of async-copied data.
🧹 Nitpick comments (1)
cpp/src/mip/presolve/third_party_presolve.cpp (1)
213-258: Significant code duplication withbuild_papilo_problem.Lines 213–258 (device-to-host copies + constraint bounds conversion) are nearly identical to lines 53–97 in
build_papilo_problem. Consider extracting a shared helper (e.g.,copy_problem_data_to_host) that returns a struct of host vectors, and call it from both functions.
|
/ok to test 9fab0d1 |
|
/ok to test 805d7bb |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mip/presolve/third_party_presolve.cpp (1)
52-81:⚠️ Potential issue | 🔴 CriticalMissing stream synchronization before host access in
build_papilo_problem.The
raft::copycalls (lines 55–75) are asynchronous onstream_view, but line 77 immediately readsh_obj_coeffson the host without synchronizing. This is the same race condition that was fixed inbuild_and_run_pslp_presolver(line 238) but was not applied here. All subsequent host accesses (h_row_types,h_constr_lb,h_bounds, etc.) are also at risk.Proposed fix
std::vector<var_t> h_var_types(var_types.size()); raft::copy(h_var_types.data(), var_types.data(), var_types.size(), stream_view); + stream_view.synchronize(); + if (maximize) { for (size_t i = 0; i < h_obj_coeffs.size(); ++i) {As per coding guidelines: "Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline" — the converse also applies: synchronization is required before host reads of async-copied buffers.
🤖 Fix all issues with AI agents
In `@cpp/src/mip/presolve/third_party_presolve.cpp`:
- Around line 339-345: The loop uses a size_t index while n_cols is an int,
risking silent wrap if n_cols is negative; in the block that negates
h_obj_coeffs when maximize is true, change the loop to use a signed index type
consistent with n_cols (e.g., for (int i = 0; i < n_cols; ++i)) or first
validate/convert n_cols to a non-negative size_t (e.g., check n_cols >= 0 then
use size_t nn = static_cast<size_t>(n_cols) and iterate with i < nn) so that
h_obj_coeffs, reduced_prob->c and the maximize branch operate over a safe,
consistently-typed range.
In `@python/cuopt/cuopt/tests/linear_programming/test_incumbent_callbacks.py`:
- Around line 92-96: The assertion is wrong because "or
MILPTerminationStatus.Optimal" is always truthy; change the check on
solution.get_termination_status() to explicitly compare against both enum values
(e.g., use "status == MILPTerminationStatus.FeasibleFound or status ==
MILPTerminationStatus.Optimal" or "status in
(MILPTerminationStatus.FeasibleFound, MILPTerminationStatus.Optimal)") so the
test actually fails on unexpected statuses; reference the existing call
solution.get_termination_status() and the enum MILPTerminationStatus when
editing the assertion.
🧹 Nitpick comments (2)
python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py (1)
577-591: No Python-level tests explicitly exercise the new PSLP presolve path.
test_dual_simplexis the only test that implicitly runs PSLP (via the default). All other tests explicitly disable presolve. Consider adding at least one test that:
- Explicitly sets
CUOPT_PRESOLVEto the PSLP value and validates numerical correctness of the postsolve solution.- Tests a non-trivial LP through the full presolve → solve → postsolve pipeline, verifying the solution against known optimal values.
This would catch regressions in the PSLP integration at the Python API level. As per coding guidelines: "Add tests for problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations."
cpp/src/mip/presolve/third_party_presolve.cpp (1)
213-258: Significant code duplication withbuild_papilo_problem(lines 52–97).The device-to-host copy block, objective negation for maximize, and constraint bound construction are nearly identical between
build_and_run_pslp_presolverandbuild_papilo_problem. Extracting a shared helper (e.g.,copy_problem_data_to_host(...)) would reduce the ~45 duplicated lines to a single call site in each builder, making future bug fixes (like the stream sync) consistent.
| std::vector<f_t> h_obj_coeffs(n_cols); | ||
| std::copy(reduced_prob->c, reduced_prob->c + n_cols, h_obj_coeffs.begin()); | ||
| if (maximize) { | ||
| for (size_t i = 0; i < n_cols; ++i) { | ||
| h_obj_coeffs[i] = -h_obj_coeffs[i]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Signed/unsigned comparison: size_t i < n_cols where n_cols is int.
n_cols is declared as int (line 313) but compared against size_t i (line 342). If n_cols were somehow negative, this would silently convert to a large unsigned value and iterate incorrectly. Use a consistent unsigned type or add a guard.
Proposed fix
- std::vector<f_t> h_obj_coeffs(n_cols);
- std::copy(reduced_prob->c, reduced_prob->c + n_cols, h_obj_coeffs.begin());
+ std::vector<f_t> h_obj_coeffs(static_cast<size_t>(n_cols));
+ std::copy(reduced_prob->c, reduced_prob->c + n_cols, h_obj_coeffs.begin());
if (maximize) {
- for (size_t i = 0; i < n_cols; ++i) {
+ for (int i = 0; i < n_cols; ++i) {
h_obj_coeffs[i] = -h_obj_coeffs[i];
}
}📝 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.
| std::vector<f_t> h_obj_coeffs(n_cols); | |
| std::copy(reduced_prob->c, reduced_prob->c + n_cols, h_obj_coeffs.begin()); | |
| if (maximize) { | |
| for (size_t i = 0; i < n_cols; ++i) { | |
| h_obj_coeffs[i] = -h_obj_coeffs[i]; | |
| } | |
| } | |
| std::vector<f_t> h_obj_coeffs(static_cast<size_t>(n_cols)); | |
| std::copy(reduced_prob->c, reduced_prob->c + n_cols, h_obj_coeffs.begin()); | |
| if (maximize) { | |
| for (int i = 0; i < n_cols; ++i) { | |
| h_obj_coeffs[i] = -h_obj_coeffs[i]; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@cpp/src/mip/presolve/third_party_presolve.cpp` around lines 339 - 345, The
loop uses a size_t index while n_cols is an int, risking silent wrap if n_cols
is negative; in the block that negates h_obj_coeffs when maximize is true,
change the loop to use a signed index type consistent with n_cols (e.g., for
(int i = 0; i < n_cols; ++i)) or first validate/convert n_cols to a non-negative
size_t (e.g., check n_cols >= 0 then use size_t nn = static_cast<size_t>(n_cols)
and iterate with i < nn) so that h_obj_coeffs, reduced_prob->c and the maximize
branch operate over a safe, consistently-typed range.
| assert ( | ||
| solution.get_termination_status() | ||
| == MILPTerminationStatus.FeasibleFound | ||
| or MILPTerminationStatus.Optimal | ||
| ) |
There was a problem hiding this comment.
Bug: or MILPTerminationStatus.Optimal is always truthy — assertion never fails.
Due to Python operator precedence, this evaluates as:
assert (status == MILPTerminationStatus.FeasibleFound) or MILPTerminationStatus.OptimalMILPTerminationStatus.Optimal is a truthy enum member, so the entire expression is always truthy regardless of the actual termination status. The test will pass even on INFEASIBLE or NUMERICAL_ISSUES.
🐛 Proposed fix
assert (
- solution.get_termination_status()
- == MILPTerminationStatus.FeasibleFound
- or MILPTerminationStatus.Optimal
+ solution.get_termination_status()
+ in (MILPTerminationStatus.FeasibleFound, MILPTerminationStatus.Optimal)
)As per coding guidelines, tests should validate numerical correctness of optimization results, not just "run without error." This assertion is effectively a no-op.
🤖 Prompt for AI Agents
In `@python/cuopt/cuopt/tests/linear_programming/test_incumbent_callbacks.py`
around lines 92 - 96, The assertion is wrong because "or
MILPTerminationStatus.Optimal" is always truthy; change the check on
solution.get_termination_status() to explicitly compare against both enum values
(e.g., use "status == MILPTerminationStatus.FeasibleFound or status ==
MILPTerminationStatus.Optimal" or "status in
(MILPTerminationStatus.FeasibleFound, MILPTerminationStatus.Optimal)") so the
test actually fails on unexpected statuses; reference the existing call
solution.get_termination_status() and the enum MILPTerminationStatus when
editing the assertion.
|
/ok to test f02c30f |
4 participants
Description
This PR adds support for using PSLP presolver for LPs. The PSLP presolver is enabled by default for LPs. This can be changed to off (--presolve 0) or switch to papilo (--presolve 1).
This PR also fixes a bug and improves performance of the folding.
Bug fix:
Coloring scheme was resulting in AX != YA because of incorrect optimization step. This is now fixed.
Performance improvement:
Replaced std::unordered_map with std::map. The unordered map required computing hash values on floating point numbers which is very slow. With this improvement dlr1 model can be folded in 6 seconds compared 30 seconds before. dlr2 can be folded now in 30 seconds.
Issue
Checklist
Summary by CodeRabbit
New Features
Refactor
API
Documentation
Tests
Chores