Write quadratic terms to mps file #949
Open
rg20 wants to merge 7 commits intoNVIDIA:release/26.04from
Open
Conversation
rg20
added
bug
📝 WalkthroughWalkthroughAdds an mps_writer overload that owns a created data_model_view from an mps_data_model; adds CSR symmetrization utilities and uses them when writing quadratic objectives; adds MPS round‑trip tests; improves presolve numeric summation; minor barrier log; replaces COO-based quadratic assembly with a COO-free CSR pipeline. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
cpp/src/barrier/barrier.cu (1)
3484-3484: Send this through debug logging instead ofprintf.Line 3484 adds solver-internal diagnostics to the normal barrier output stream. That changes the default log format for every solve; please gate it behind
settings.log.debug(...)or a debug flag instead.Suggested change
- settings.log.printf("norm_b: %.2e, norm_c: %.2e\n", norm_b, norm_c); + settings.log.debug("norm_b: %.2e, norm_c: %.2e\n", norm_b, norm_c);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/barrier/barrier.cu` at line 3484, The diagnostic printf call settings.log.printf("norm_b: %.2e, norm_c: %.2e\n", norm_b, norm_c) should be moved to debug logging so it doesn't alter normal output; replace or wrap it with the logger's debug method (e.g., settings.log.debug(...)) or conditionally emit it only when a debug flag is enabled, keeping the same formatted message and variable references (norm_b, norm_c) so the diagnostic remains available but only in debug mode.cpp/src/dual_simplex/presolve.cpp (1)
1504-1512: Consider adding a defensive assertion for size consistency.The loop iterates over
input_x.size()but accessesremoved_lower_bounds[j]. While the current code flow ensures these sizes match after variable restoration, adding an explicit assertion would guard against future refactoring that might break this invariant.🛡️ Suggested defensive assertion
if (presolve_info.removed_lower_bounds.size() > 0) { i_t num_lower_bounds = 0; + assert(input_x.size() == presolve_info.removed_lower_bounds.size()); // We removed some lower bounds so we need to map the crushed solution back to the original // variables for (i_t j = 0; j < input_x.size(); j++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/dual_simplex/presolve.cpp` around lines 1504 - 1512, Before iterating to restore removed lower bounds, add a defensive assertion that input_x.size() equals presolve_info.removed_lower_bounds.size() to catch future regressions; place it just before the loop that references input_x and presolve_info.removed_lower_bounds (the block that computes num_lower_bounds, updates input_x[j], and calls settings.log.printf) so the code verifies size consistency before accessing removed_lower_bounds[j].cpp/src/utilities/sparse_matrix_helpers.hpp (1)
35-119: Significant code duplication withsparse_matrix_utils.hpp.This implementation is nearly identical to
cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp. Consider consolidating into a single shared utility to avoid maintenance burden and potential divergence.Additionally, the comment at line 44 says "Optimized 2-pass algorithm" but the implementation is actually a 3-pass algorithm (count → fill → deduplicate).
💡 Comment fix
- // Optimized 2-pass algorithm (no COO intermediate) + // Optimized 3-pass algorithm (no COO intermediate)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/utilities/sparse_matrix_helpers.hpp` around lines 35 - 119, The function symmetrize_csr is duplicated vs cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp and its header comment incorrectly says "Optimized 2-pass algorithm" though the code uses three passes (count → fill → deduplicate); consolidate by moving the implementation of symmetrize_csr (and its helper locals: temp_offsets, temp_indices, temp_values, row_pos, workspace, out_offsets/out_indices/out_values handling) into a single shared utility header/source used by both consumers (create a common utilities header/namespace, replace the duplicate definitions with an include or call to that single symmetrize_csr), update callers to include the shared header, remove the redundant implementation, and correct the top comment to "Optimized 3-pass algorithm (count → fill → deduplicate)".cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp (1)
32-50: Guard againstn_rows <= 0in pointer-based overload.If
n_rowsis zero or negative, the function will still allocate vectors and attempt iteration, which could lead to unexpected behavior.🛡️ Proposed early-return guard
template <typename i_t, typename f_t> void symmetrize_csr(const f_t* in_values, const i_t* in_indices, const i_t* in_offsets, i_t n_rows, std::vector<f_t>& out_values, std::vector<i_t>& out_indices, std::vector<i_t>& out_offsets) { + if (n_rows <= 0) { + out_values.clear(); + out_indices.clear(); + out_offsets.assign(1, 0); + return; + } // Optimized 3-pass algorithm // Pass 1: Count entries per row in A + A^T🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp` around lines 32 - 50, The symmetrize_csr function lacks a guard for non-positive n_rows and may dereference in_offsets/in_indices; at the top of symmetrize_csr add an early return for n_rows <= 0 that clears out_values and out_indices and sets out_offsets to an empty vector (or a single 0 element if callers expect offsets.size() == 1), then return immediately to avoid accessing in_offsets[0] or allocating row_counts; this prevents any further loops or allocations (references: symmetrize_csr, in_offsets, out_offsets, row_counts).cpp/src/pdlp/optimization_problem.cu (1)
18-18: Consider using thesymmetrize_csrutility instead of duplicating the algorithm.The
sparse_matrix_helpers.hppheader is included at line 18, but instead of callingcuopt::symmetrize_csr(), the same 3-pass algorithm is duplicated inline. This creates maintenance burden and risks divergence.Also, the comment at line 192 says "2-pass algorithm" but it's actually 3-pass.
♻️ Proposed refactor using the utility
- // Build Q + Q^T using optimized 2-pass algorithm (no COO intermediate) - // Memory: ~3× nnz, ~2x faster than original COO-based approach - i_t qn = size_offsets - 1; // Number of variables - - // Pass 1: Count entries per row in Q + Q^T - std::vector<i_t> row_counts(qn, 0); - ... (entire algorithm) - Q_values_.resize(nz); + // Build Q + Q^T using optimized 3-pass algorithm (no COO intermediate) + std::vector<f_t> in_values(Q_values, Q_values + size_values); + std::vector<i_t> in_indices(Q_indices, Q_indices + size_indices); + std::vector<i_t> in_offsets(Q_offsets, Q_offsets + size_offsets); + + cuopt::symmetrize_csr(in_values, in_indices, in_offsets, Q_values_, Q_indices_, Q_offsets_);Also applies to: 192-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/pdlp/optimization_problem.cu` at line 18, Replace the duplicated 3-pass CSR symmetrization code with a call to the existing utility cuopt::symmetrize_csr(), removing the inline algorithm between the current symmetrization start and end (the block flagged around lines 192-267) and pass the same CSR inputs/outputs (row_ptr, col_idx, values or their device pointers) used by the duplicated code; ensure the call signature matches cuopt::symmetrize_csr(row_ptr_in, col_idx_in, vals_in, nnz_in, row_ptr_out, col_idx_out, vals_out, allocator/stream arguments) and adapt variable names accordingly, then update the nearby comment that currently reads "2-pass algorithm" to correctly reflect "3-pass algorithm" or remove it if no longer needed.cpp/libmps_parser/tests/mps_parser_test.cpp (1)
1022-1043: Consider using a cross-platform temp file mechanism and RAII for cleanup.The tests use hardcoded
/tmp/paths which may not work on all platforms. Additionally, if a test fails before reachingstd::filesystem::remove(), the temp file will remain.Consider using
std::filesystem::temp_directory_path()and a RAII wrapper or test fixture for automatic cleanup.💡 Example improvement
TEST(mps_roundtrip, linear_programming_basic) { std::string input_file = cuopt::test::get_rapids_dataset_root_dir() + "/linear_programming/good-mps-1.mps"; - std::string temp_file = "/tmp/mps_roundtrip_lp_test.mps"; + std::string temp_file = + (std::filesystem::temp_directory_path() / "mps_roundtrip_lp_test.mps").string(); + + // RAII cleanup + struct TempFileGuard { + std::string path; + ~TempFileGuard() { std::filesystem::remove(path); } + } guard{temp_file}; // Read original auto original = parse_mps<int, double>(input_file, true); // ... rest of test - // Cleanup - std::filesystem::remove(temp_file); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/tests/mps_parser_test.cpp` around lines 1022 - 1043, Replace the hardcoded "/tmp/mps_roundtrip_lp_test.mps" with a platform-safe temp path (use std::filesystem::temp_directory_path() combined with std::filesystem::unique_path, e.g. unique_path("mps_roundtrip_%%%%-%%%%.mps")) inside the TEST(mps_roundtrip, linear_programming_basic) test; create a small RAII helper (e.g. TempFile or use a test fixture with a destructor/TearDown) that holds the std::filesystem::path and removes the file in its destructor, then use that temp path when calling mps_writer_t<int,double>::write and parse_mps<int,double> so cleanup always runs even on failures.
🤖 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/libmps_parser/src/utilities/sparse_matrix_utils.hpp`:
- Around line 119-135: The wrapper symmetrize_csr must handle an empty
in_offsets to avoid underflow when computing n_rows; add a guard at the start of
the function that checks if in_offsets.empty() and if so clear out_values and
out_indices and set out_offsets to a single element {0} (or otherwise initialize
outputs to represent an empty CSR) then return early; otherwise proceed to
compute n_rows = in_offsets.size() - 1 and call the underlying symmetrize_csr
overload as before.
In `@cpp/src/utilities/sparse_matrix_helpers.hpp`:
- Around line 136-152: The wrapper symmetrize_csr currently computes n_rows =
in_offsets.size() - 1 which underflows when in_offsets is empty; add a guard at
the top of the function to handle the empty-input case (e.g., if
in_offsets.empty() ) and either return early after clearing/setting out_values,
out_indices, and out_offsets to empty/default state or set n_rows = 0 before
calling the lower-level symmetrize_csr overload; update the function
symmetrize_csr(const std::vector<f_t>&,... ) to use this guard so it never
performs size()-1 on an empty in_offsets.
---
Nitpick comments:
In `@cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp`:
- Around line 32-50: The symmetrize_csr function lacks a guard for non-positive
n_rows and may dereference in_offsets/in_indices; at the top of symmetrize_csr
add an early return for n_rows <= 0 that clears out_values and out_indices and
sets out_offsets to an empty vector (or a single 0 element if callers expect
offsets.size() == 1), then return immediately to avoid accessing in_offsets[0]
or allocating row_counts; this prevents any further loops or allocations
(references: symmetrize_csr, in_offsets, out_offsets, row_counts).
In `@cpp/libmps_parser/tests/mps_parser_test.cpp`:
- Around line 1022-1043: Replace the hardcoded "/tmp/mps_roundtrip_lp_test.mps"
with a platform-safe temp path (use std::filesystem::temp_directory_path()
combined with std::filesystem::unique_path, e.g.
unique_path("mps_roundtrip_%%%%-%%%%.mps")) inside the TEST(mps_roundtrip,
linear_programming_basic) test; create a small RAII helper (e.g. TempFile or use
a test fixture with a destructor/TearDown) that holds the std::filesystem::path
and removes the file in its destructor, then use that temp path when calling
mps_writer_t<int,double>::write and parse_mps<int,double> so cleanup always runs
even on failures.
In `@cpp/src/barrier/barrier.cu`:
- Line 3484: The diagnostic printf call settings.log.printf("norm_b: %.2e,
norm_c: %.2e\n", norm_b, norm_c) should be moved to debug logging so it doesn't
alter normal output; replace or wrap it with the logger's debug method (e.g.,
settings.log.debug(...)) or conditionally emit it only when a debug flag is
enabled, keeping the same formatted message and variable references (norm_b,
norm_c) so the diagnostic remains available but only in debug mode.
In `@cpp/src/dual_simplex/presolve.cpp`:
- Around line 1504-1512: Before iterating to restore removed lower bounds, add a
defensive assertion that input_x.size() equals
presolve_info.removed_lower_bounds.size() to catch future regressions; place it
just before the loop that references input_x and
presolve_info.removed_lower_bounds (the block that computes num_lower_bounds,
updates input_x[j], and calls settings.log.printf) so the code verifies size
consistency before accessing removed_lower_bounds[j].
In `@cpp/src/pdlp/optimization_problem.cu`:
- Line 18: Replace the duplicated 3-pass CSR symmetrization code with a call to
the existing utility cuopt::symmetrize_csr(), removing the inline algorithm
between the current symmetrization start and end (the block flagged around lines
192-267) and pass the same CSR inputs/outputs (row_ptr, col_idx, values or their
device pointers) used by the duplicated code; ensure the call signature matches
cuopt::symmetrize_csr(row_ptr_in, col_idx_in, vals_in, nnz_in, row_ptr_out,
col_idx_out, vals_out, allocator/stream arguments) and adapt variable names
accordingly, then update the nearby comment that currently reads "2-pass
algorithm" to correctly reflect "3-pass algorithm" or remove it if no longer
needed.
In `@cpp/src/utilities/sparse_matrix_helpers.hpp`:
- Around line 35-119: The function symmetrize_csr is duplicated vs
cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp and its header comment
incorrectly says "Optimized 2-pass algorithm" though the code uses three passes
(count → fill → deduplicate); consolidate by moving the implementation of
symmetrize_csr (and its helper locals: temp_offsets, temp_indices, temp_values,
row_pos, workspace, out_offsets/out_indices/out_values handling) into a single
shared utility header/source used by both consumers (create a common utilities
header/namespace, replace the duplicate definitions with an include or call to
that single symmetrize_csr), update callers to include the shared header, remove
the redundant implementation, and correct the top comment to "Optimized 3-pass
algorithm (count → fill → deduplicate)".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f4d3fea-12c6-4d00-a3ec-893c0132ba72
📒 Files selected for processing (8)
cpp/libmps_parser/include/mps_parser/mps_writer.hppcpp/libmps_parser/src/mps_writer.cppcpp/libmps_parser/src/utilities/sparse_matrix_utils.hppcpp/libmps_parser/tests/mps_parser_test.cppcpp/src/barrier/barrier.cucpp/src/dual_simplex/presolve.cppcpp/src/pdlp/optimization_problem.cucpp/src/utilities/sparse_matrix_helpers.hpp
Comment on lines
+119
to
+135
| template <typename i_t, typename f_t> | ||
| void symmetrize_csr(const std::vector<f_t>& in_values, | ||
| const std::vector<i_t>& in_indices, | ||
| const std::vector<i_t>& in_offsets, | ||
| std::vector<f_t>& out_values, | ||
| std::vector<i_t>& out_indices, | ||
| std::vector<i_t>& out_offsets) | ||
| { | ||
| i_t n_rows = static_cast<i_t>(in_offsets.size()) - 1; | ||
| symmetrize_csr(in_values.data(), | ||
| in_indices.data(), | ||
| in_offsets.data(), | ||
| n_rows, | ||
| out_values, | ||
| out_indices, | ||
| out_offsets); | ||
| } |
There was a problem hiding this comment.
Handle edge case: empty in_offsets vector.
When in_offsets is empty, in_offsets.size() - 1 will underflow (for unsigned) or produce an unexpected value. Add a guard for this edge case.
🛡️ Proposed fix
template <typename i_t, typename f_t>
void symmetrize_csr(const std::vector<f_t>& in_values,
const std::vector<i_t>& in_indices,
const std::vector<i_t>& in_offsets,
std::vector<f_t>& out_values,
std::vector<i_t>& out_indices,
std::vector<i_t>& out_offsets)
{
+ if (in_offsets.empty()) {
+ out_values.clear();
+ out_indices.clear();
+ out_offsets.clear();
+ return;
+ }
i_t n_rows = static_cast<i_t>(in_offsets.size()) - 1;
symmetrize_csr(in_values.data(),
in_indices.data(),
in_offsets.data(),
n_rows,
out_values,
out_indices,
out_offsets);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp` around lines 119 -
135, The wrapper symmetrize_csr must handle an empty in_offsets to avoid
underflow when computing n_rows; add a guard at the start of the function that
checks if in_offsets.empty() and if so clear out_values and out_indices and set
out_offsets to a single element {0} (or otherwise initialize outputs to
represent an empty CSR) then return early; otherwise proceed to compute n_rows =
in_offsets.size() - 1 and call the underlying symmetrize_csr overload as before.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/src/utilities/sparse_matrix_helpers.hpp (1)
144-144:⚠️ Potential issue | 🟡 MinorEarly return leaves output vectors in an undefined state.
When
in_offsets.size() <= 1, the function returns without modifyingout_values,out_indices, orout_offsets. If these vectors contain stale data from a previous call, the caller might incorrectly interpret that data as valid output.Consider clearing the output vectors to represent an empty symmetric matrix:
🛠️ Suggested fix
if (in_offsets.size() <= 1) { + out_values.clear(); + out_indices.clear(); + out_offsets.assign(1, 0); // Single offset of 0 for empty matrix return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/utilities/sparse_matrix_helpers.hpp` at line 144, The early-return branch when in_offsets.size() <= 1 leaves out_values, out_indices, and out_offsets unchanged; modify the function in sparse_matrix_helpers.hpp so that before returning it explicitly clears out_values.clear(), out_indices.clear(), and out_offsets.clear() (or assigns empty vectors) to represent an empty symmetric matrix, then return; this ensures no stale data is left in those output containers.
🤖 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/utilities/sparse_matrix_helpers.hpp`:
- Around line 91-112: The workspace sentinel of -1 is invalid for unsigned i_t
and causes incorrect behavior; replace the sentinel with a portable sentinel
like const i_t SENTINEL = std::numeric_limits<i_t>::max() (include <limits>) and
initialize workspace(n_rows, SENTINEL), then change the logic to test
workspace[j] != SENTINEL (or compare to SENTINEL) instead of workspace[j] >=
row_start_out and when first visiting set workspace[j] = nz; alternatively, if
you prefer stricter typing, add static_assert(std::is_signed<i_t>::value, "i_t
must be signed") near the template to enforce signed indices.
---
Duplicate comments:
In `@cpp/src/utilities/sparse_matrix_helpers.hpp`:
- Line 144: The early-return branch when in_offsets.size() <= 1 leaves
out_values, out_indices, and out_offsets unchanged; modify the function in
sparse_matrix_helpers.hpp so that before returning it explicitly clears
out_values.clear(), out_indices.clear(), and out_offsets.clear() (or assigns
empty vectors) to represent an empty symmetric matrix, then return; this ensures
no stale data is left in those output containers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb2c081c-b0b5-45de-b197-dd2a32275b5f
📒 Files selected for processing (1)
cpp/src/utilities/sparse_matrix_helpers.hpp
Comment on lines
+91
to
+112
| std::vector<i_t> workspace(n_rows, -1); | ||
| out_offsets.resize(n_rows + 1); | ||
| out_indices.resize(total_entries); | ||
| out_values.resize(total_entries); | ||
|
|
||
| i_t nz = 0; | ||
| for (i_t i = 0; i < n_rows; ++i) { | ||
| i_t row_start_out = nz; | ||
| out_offsets[i] = row_start_out; | ||
|
|
||
| for (i_t p = temp_offsets[i]; p < temp_offsets[i + 1]; ++p) { | ||
| i_t j = temp_indices[p]; | ||
| f_t x = temp_values[p]; | ||
|
|
||
| if (workspace[j] >= row_start_out) { | ||
| out_values[workspace[j]] += x; | ||
| } else { | ||
| workspace[j] = nz; | ||
| out_indices[nz] = j; | ||
| out_values[nz] = x; | ||
| nz++; | ||
| } |
There was a problem hiding this comment.
Workspace initialization is incompatible with unsigned index types.
Line 91 initializes workspace with -1, and line 105 uses workspace[j] >= row_start_out to detect unvisited columns. If i_t is an unsigned type (e.g., uint32_t), -1 wraps to the maximum value, which will always be >= row_start_out, causing incorrect value accumulation on unvisited entries.
Consider using a sentinel that works for both signed and unsigned types, or add a static_assert to enforce signed index types.
🛠️ Suggested fix using a dedicated visited tracking approach
// Pass 3: Deduplicate and build final CSR
- std::vector<i_t> workspace(n_rows, -1);
+ // Use a separate flag array for visited tracking to support unsigned i_t
+ std::vector<i_t> workspace(n_rows);
+ std::vector<bool> visited(n_rows, false);
out_offsets.resize(n_rows + 1);
out_indices.resize(total_entries);
out_values.resize(total_entries);
i_t nz = 0;
for (i_t i = 0; i < n_rows; ++i) {
i_t row_start_out = nz;
out_offsets[i] = row_start_out;
for (i_t p = temp_offsets[i]; p < temp_offsets[i + 1]; ++p) {
i_t j = temp_indices[p];
f_t x = temp_values[p];
- if (workspace[j] >= row_start_out) {
+ if (visited[j]) {
out_values[workspace[j]] += x;
} else {
+ visited[j] = true;
workspace[j] = nz;
out_indices[nz] = j;
out_values[nz] = x;
nz++;
}
}
+ // Reset visited flags for columns in this row
+ for (i_t p = out_offsets[i]; p < nz; ++p) {
+ visited[out_indices[p]] = false;
+ }
}Alternatively, enforce signed types:
static_assert(std::is_signed<i_t>::value, "i_t must be a signed integer type for workspace sentinel");📝 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
| std::vector<i_t> workspace(n_rows, -1); | |
| out_offsets.resize(n_rows + 1); | |
| out_indices.resize(total_entries); | |
| out_values.resize(total_entries); | |
| i_t nz = 0; | |
| for (i_t i = 0; i < n_rows; ++i) { | |
| i_t row_start_out = nz; | |
| out_offsets[i] = row_start_out; | |
| for (i_t p = temp_offsets[i]; p < temp_offsets[i + 1]; ++p) { | |
| i_t j = temp_indices[p]; | |
| f_t x = temp_values[p]; | |
| if (workspace[j] >= row_start_out) { | |
| out_values[workspace[j]] += x; | |
| } else { | |
| workspace[j] = nz; | |
| out_indices[nz] = j; | |
| out_values[nz] = x; | |
| nz++; | |
| } | |
| // Use a separate flag array for visited tracking to support unsigned i_t | |
| std::vector<i_t> workspace(n_rows); | |
| std::vector<bool> visited(n_rows, false); | |
| out_offsets.resize(n_rows + 1); | |
| out_indices.resize(total_entries); | |
| out_values.resize(total_entries); | |
| i_t nz = 0; | |
| for (i_t i = 0; i < n_rows; ++i) { | |
| i_t row_start_out = nz; | |
| out_offsets[i] = row_start_out; | |
| for (i_t p = temp_offsets[i]; p < temp_offsets[i + 1]; ++p) { | |
| i_t j = temp_indices[p]; | |
| f_t x = temp_values[p]; | |
| if (visited[j]) { | |
| out_values[workspace[j]] += x; | |
| } else { | |
| visited[j] = true; | |
| workspace[j] = nz; | |
| out_indices[nz] = j; | |
| out_values[nz] = x; | |
| nz++; | |
| } | |
| } | |
| // Reset visited flags for columns in this row | |
| for (i_t p = out_offsets[i]; p < nz; ++p) { | |
| visited[out_indices[p]] = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/utilities/sparse_matrix_helpers.hpp` around lines 91 - 112, The
workspace sentinel of -1 is invalid for unsigned i_t and causes incorrect
behavior; replace the sentinel with a portable sentinel like const i_t SENTINEL
= std::numeric_limits<i_t>::max() (include <limits>) and initialize
workspace(n_rows, SENTINEL), then change the logic to test workspace[j] !=
SENTINEL (or compare to SENTINEL) instead of workspace[j] >= row_start_out and
when first visiting set workspace[j] = nz; alternatively, if you prefer stricter
typing, add static_assert(std::is_signed<i_t>::value, "i_t must be signed") near
the template to enforce signed indices.
rg20
force-pushed
the
qp_improvements_new
branch
from
8ff59e3 to
1b94b55
Compare
Contributor
Author
|
/ok to test 1b94b55 |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/libmps_parser/include/mps_parser/mps_writer.hpp (1)
44-60:⚠️ Potential issue | 🔴 CriticalAvoid dangling
problem_when constructed from temporary data modelsThis overload stores a view reference, but not guaranteed ownership of the model memory the view points to. A temporary
mps_data_model_tcan leaveproblem_dangling after constructor return.As per coding guidelines, "Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results."🔧 Proposed fix
class mps_writer_t { public: - mps_writer_t(const mps_data_model_t<i_t, f_t>& problem); + mps_writer_t(mps_data_model_t<i_t, f_t> problem); ... private: + std::unique_ptr<mps_data_model_t<i_t, f_t>> owned_model_; std::unique_ptr<data_model_view_t<i_t, f_t>> owned_view_; const data_model_view_t<i_t, f_t>& problem_;-mps_writer_t<i_t, f_t>::mps_writer_t(const mps_data_model_t<i_t, f_t>& problem) - : owned_view_(std::make_unique<data_model_view_t<i_t, f_t>>(create_view(problem))), +mps_writer_t<i_t, f_t>::mps_writer_t(mps_data_model_t<i_t, f_t> problem) + : owned_model_(std::make_unique<mps_data_model_t<i_t, f_t>>(std::move(problem))), + owned_view_(std::make_unique<data_model_view_t<i_t, f_t>>(create_view(*owned_model_))), problem_(*owned_view_) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/include/mps_parser/mps_writer.hpp` around lines 44 - 60, The constructor mps_writer_t(const mps_data_model_t<i_t, f_t>& problem) can leave problem_ dangling when called with a temporary; fix by changing the constructor to take the model by value (mps_data_model_t<i_t, f_t> model), then always build an owned view via owned_view_ = std::make_unique<data_model_view_t<i_t,f_t>>(create_view(model)) and set problem_ to reference *owned_view_; keep the static helper create_view and ensure other call sites are updated to pass by value (or std::move) so problem_ never refers to memory that may be destroyed.
🧹 Nitpick comments (3)
cpp/src/dual_simplex/presolve.cpp (1)
1503-1512: Consider adding a defensive assertion for bounds safety.The loop at line 1508 iterates over
input_x.size()while indexing intoremoved_lower_bounds. While the current logic ensures these sizes match (both should equal the originalnum_colsafter uncrush operations), an explicit assertion would guard against future refactoring errors.🛡️ Proposed defensive check
if (presolve_info.removed_lower_bounds.size() > 0) { i_t num_lower_bounds = 0; + assert(input_x.size() <= presolve_info.removed_lower_bounds.size()); // We removed some lower bounds so we need to map the crushed solution back to the original // variables for (i_t j = 0; j < input_x.size(); j++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/dual_simplex/presolve.cpp` around lines 1503 - 1512, Add a defensive assertion before the loop that maps crushed solution back to original variables to ensure presolve_info.removed_lower_bounds.size() matches input_x.size(); specifically check the sizes of removed_lower_bounds and input_x (and/or original num_cols if available) and fail fast if they differ so indexing in the loop that references presolve_info.removed_lower_bounds[j] is safe; place this assertion just above the for-loop that updates input_x and before the settings.log.printf call.cpp/libmps_parser/tests/mps_parser_test.cpp (1)
903-1020: Broaden round-trip comparison coverage
compare_data_models(...)currently skips key metadata checks (e.g., sense, objective name/offset/scaling, variable/row types, names). That can miss regressions while matrix/bounds still match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/tests/mps_parser_test.cpp` around lines 903 - 1020, The compare_data_models function is missing checks for model metadata; add assertions to compare objective sense (use original.get_sense()), objective name/offset/scaling (get_objective_name(), get_objective_offset(), get_objective_scaling() — use EXPECT_NEAR for numeric offset/scaling), variable and row types (get_variable_types(), get_row_types() with EXPECT_EQ), and variable/constraint names (get_variable_names(), get_constraint_names() with EXPECT_EQ); place these checks near the top after dimensions and use the same tol for numeric comparisons so round-trip mismatches are detected.cpp/src/utilities/sparse_matrix_helpers.hpp (1)
44-45: Comment inaccuracy: states "2-pass" but algorithm has 3 passes.The comment says "Optimized 2-pass algorithm" but the implementation has three distinct passes: (1) count entries, (2) fill temporary storage, and (3) deduplicate into final CSR.
📝 Proposed fix
- // Optimized 2-pass algorithm (no COO intermediate) - // Memory: ~3× nnz, ~2x faster than COO-based approach + // Optimized 3-pass algorithm (no COO intermediate) + // Memory: ~3× nnz, faster than COO-based approach🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/utilities/sparse_matrix_helpers.hpp` around lines 44 - 45, The header comment in sparse_matrix_helpers.hpp incorrectly labels the routine as an "Optimized 2-pass algorithm" despite the implementation performing three passes (count entries, fill temporary storage, deduplicate into final CSR); update that comment string to "Optimized 3-pass algorithm" (or reword to enumerate the three passes), and adjust any accompanying wording (e.g., memory/time claim) to remain accurate; locate the exact comment text "Optimized 2-pass algorithm (no COO intermediate)" and change it to reflect the three-pass implementation.
🤖 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/libmps_parser/src/mps_writer.cpp`:
- Around line 43-50: The code currently skips calling
view.set_csr_constraint_matrix when A_values.empty(), which breaks zero-nnz CSR
matrices because write() later accesses constraint_matrix_offsets[row_id + 1];
instead ensure set_csr_constraint_matrix is invoked even when A_values is empty
by removing the A_values.empty() guard and calling
view.set_csr_constraint_matrix with the existing A_offsets (and
A_values/A_indices pointers with size 0 as appropriate) whenever the constraint
matrix structure (A_offsets) is present or the matrix is constrained; update the
call site (the block around view.set_csr_constraint_matrix) to pass correct zero
sizes (static_cast<i_t>(0)) for values/indices when A_values/A_indices are empty
so constraint_matrix_offsets is always initialized for write().
In `@cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp`:
- Around line 86-108: The workspace sentinel uses -1 which breaks when i_t is
unsigned; add a compile-time guard to require signed index types by inserting
static_assert(std::is_signed<i_t>::value, "i_t must be a signed integer type");
near the top of the function/template (before workspace is initialized) so
workspace, temp_offsets, temp_indices and related logic remain correct for all
builds; alternatively, replace the -1 sentinel with a sentinel value based on
std::numeric_limits<i_t>::min() and adjust the visited check accordingly if you
prefer a runtime-compatible sentinel instead of the static_assert.
In `@cpp/libmps_parser/tests/mps_parser_test.cpp`:
- Around line 1024-1128: The tests (e.g., TEST
mps_roundtrip::linear_programming, linear_programming_with_bounds,
quadratic_programming_qp_test_1/2) currently use fixed temp_file paths like
"/tmp/mps_roundtrip_*.mps" and call std::filesystem::remove(...) manually, which
can collide in parallel runs and won't run if a test ASSERT_* aborts; change to
generate unique temp paths (use std::filesystem::temp_directory_path() +
std::filesystem::unique_path() or similar) and manage removal with an RAII
file-cleanup helper (a small scope-local class that holds the temp filename and
deletes it in its destructor); update usages where temp_file and
writer.write(temp_file) occur and remove the manual std::filesystem::remove(...)
calls so cleanup always runs even on early returns.
In `@cpp/src/pdlp/optimization_problem.cu`:
- Around line 197-205: The code currently indexes Q_indices and uses each j to
update row_counts[j], row_pos[j], and workspace[j] without bounds checks; add
validation that each j read from Q_indices[p] is within [0, qn) before any write
or increment and handle violations by returning an error or failing early (e.g.,
validate inside the loops in the Pass 1 and Pass 2 blocks that process
Q_offsets/Q_indices and bail with a clear error code/message if j < 0 || j >=
qn); also validate Q_offsets array shape (size == qn+1 and non-decreasing)
before iterating to avoid out-of-bounds reads when computing ranges used by
functions that manipulate row_counts, row_pos, and workspace.
- Around line 207-216: The CSR size accumulation currently uses i_t
(temp_offsets, total_entries) which can overflow for large qn; change the
accumulation and capacity checks to use a wider unsigned/signed type (e.g.,
size_t or int64_t) when computing temp_offsets and total_entries, validate that
total_entries fits in size_t and is non-negative before allocating
temp_indices/temp_values, and add an explicit overflow check when summing
row_counts (e.g., detect if temp_offsets[i]+row_counts[i] < temp_offsets[i] or
exceeds a max allowed size) to early-return/error; apply the same change to the
other accumulation block handling temp_offsets/total_entries around the later
code paths that build CSR (functions/variables: temp_offsets, total_entries,
temp_indices, temp_values, row_counts, qn, i_t).
In `@cpp/src/utilities/sparse_matrix_helpers.hpp`:
- Line 144: The early return when in_offsets.size() <= 1 leaves output vectors
uninitialized; update the function that takes in_offsets and writes to
out_offsets, out_col_indices and out_values so that before returning it sets
out_offsets to a valid empty CSR (e.g., out_offsets = {0}) and clears
out_col_indices and out_values (or otherwise initializes them to empty
consistent CSR state), then return; modify the branch around the check of
in_offsets.size() to perform these assignments to ensure callers receive a
well-defined empty matrix.
---
Outside diff comments:
In `@cpp/libmps_parser/include/mps_parser/mps_writer.hpp`:
- Around line 44-60: The constructor mps_writer_t(const mps_data_model_t<i_t,
f_t>& problem) can leave problem_ dangling when called with a temporary; fix by
changing the constructor to take the model by value (mps_data_model_t<i_t, f_t>
model), then always build an owned view via owned_view_ =
std::make_unique<data_model_view_t<i_t,f_t>>(create_view(model)) and set
problem_ to reference *owned_view_; keep the static helper create_view and
ensure other call sites are updated to pass by value (or std::move) so problem_
never refers to memory that may be destroyed.
---
Nitpick comments:
In `@cpp/libmps_parser/tests/mps_parser_test.cpp`:
- Around line 903-1020: The compare_data_models function is missing checks for
model metadata; add assertions to compare objective sense (use
original.get_sense()), objective name/offset/scaling (get_objective_name(),
get_objective_offset(), get_objective_scaling() — use EXPECT_NEAR for numeric
offset/scaling), variable and row types (get_variable_types(), get_row_types()
with EXPECT_EQ), and variable/constraint names (get_variable_names(),
get_constraint_names() with EXPECT_EQ); place these checks near the top after
dimensions and use the same tol for numeric comparisons so round-trip mismatches
are detected.
In `@cpp/src/dual_simplex/presolve.cpp`:
- Around line 1503-1512: Add a defensive assertion before the loop that maps
crushed solution back to original variables to ensure
presolve_info.removed_lower_bounds.size() matches input_x.size(); specifically
check the sizes of removed_lower_bounds and input_x (and/or original num_cols if
available) and fail fast if they differ so indexing in the loop that references
presolve_info.removed_lower_bounds[j] is safe; place this assertion just above
the for-loop that updates input_x and before the settings.log.printf call.
In `@cpp/src/utilities/sparse_matrix_helpers.hpp`:
- Around line 44-45: The header comment in sparse_matrix_helpers.hpp incorrectly
labels the routine as an "Optimized 2-pass algorithm" despite the implementation
performing three passes (count entries, fill temporary storage, deduplicate into
final CSR); update that comment string to "Optimized 3-pass algorithm" (or
reword to enumerate the three passes), and adjust any accompanying wording
(e.g., memory/time claim) to remain accurate; locate the exact comment text
"Optimized 2-pass algorithm (no COO intermediate)" and change it to reflect the
three-pass implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22c4510c-8c88-4937-a3e7-1e083243ac57
📒 Files selected for processing (8)
cpp/libmps_parser/include/mps_parser/mps_writer.hppcpp/libmps_parser/src/mps_writer.cppcpp/libmps_parser/src/utilities/sparse_matrix_utils.hppcpp/libmps_parser/tests/mps_parser_test.cppcpp/src/barrier/barrier.cucpp/src/dual_simplex/presolve.cppcpp/src/pdlp/optimization_problem.cucpp/src/utilities/sparse_matrix_helpers.hpp
Comment on lines
+43
to
+50
| if (!A_values.empty()) { | ||
| view.set_csr_constraint_matrix(A_values.data(), | ||
| static_cast<i_t>(A_values.size()), | ||
| A_indices.data(), | ||
| static_cast<i_t>(A_indices.size()), | ||
| A_offsets.data(), | ||
| static_cast<i_t>(A_offsets.size())); | ||
| } |
There was a problem hiding this comment.
Don’t skip CSR setup when A_values is empty
Line 43 should not gate on values. A valid zero-nnz CSR still needs offsets; otherwise write() can read constraint_matrix_offsets[row_id + 1] out of bounds for constrained-but-empty matrices.
✅ Proposed fix
- if (!A_values.empty()) {
+ if (!A_offsets.empty()) {
view.set_csr_constraint_matrix(A_values.data(),
static_cast<i_t>(A_values.size()),
A_indices.data(),
static_cast<i_t>(A_indices.size()),
A_offsets.data(),
static_cast<i_t>(A_offsets.size()));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/libmps_parser/src/mps_writer.cpp` around lines 43 - 50, The code
currently skips calling view.set_csr_constraint_matrix when A_values.empty(),
which breaks zero-nnz CSR matrices because write() later accesses
constraint_matrix_offsets[row_id + 1]; instead ensure set_csr_constraint_matrix
is invoked even when A_values is empty by removing the A_values.empty() guard
and calling view.set_csr_constraint_matrix with the existing A_offsets (and
A_values/A_indices pointers with size 0 as appropriate) whenever the constraint
matrix structure (A_offsets) is present or the matrix is constrained; update the
call site (the block around view.set_csr_constraint_matrix) to pass correct zero
sizes (static_cast<i_t>(0)) for values/indices when A_values/A_indices are empty
so constraint_matrix_offsets is always initialized for write().
Comment on lines
+86
to
+108
| std::vector<i_t> workspace(n_rows, -1); | ||
| out_offsets.resize(n_rows + 1); | ||
| out_indices.resize(total_entries); | ||
| out_values.resize(total_entries); | ||
|
|
||
| i_t nz = 0; | ||
| for (i_t i = 0; i < n_rows; ++i) { | ||
| i_t row_start_out = nz; | ||
| out_offsets[i] = row_start_out; | ||
|
|
||
| for (i_t p = temp_offsets[i]; p < temp_offsets[i + 1]; ++p) { | ||
| i_t j = temp_indices[p]; | ||
| f_t x = temp_values[p]; | ||
|
|
||
| if (workspace[j] >= row_start_out) { | ||
| out_values[workspace[j]] += x; | ||
| } else { | ||
| workspace[j] = nz; | ||
| out_indices[nz] = j; | ||
| out_values[nz] = x; | ||
| nz++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Workspace sentinel -1 breaks for unsigned index types.
The workspace is initialized with -1 (line 86), and the check at line 100 uses workspace[j] >= row_start_out to detect whether a column was already visited in the current row. If i_t is an unsigned type (e.g., uint32_t), -1 wraps to std::numeric_limits<i_t>::max(), which will always satisfy >= row_start_out, causing incorrect value accumulation on unvisited entries.
Either enforce signed types via static_assert or use a sentinel that works for both:
🛠️ Proposed fix using explicit sentinel
+#include <limits>
+#include <type_traits>
+
// Pass 3: Deduplicate and build final CSR
- std::vector<i_t> workspace(n_rows, -1);
+ constexpr i_t SENTINEL = std::is_signed<i_t>::value
+ ? static_cast<i_t>(-1)
+ : std::numeric_limits<i_t>::max();
+ std::vector<i_t> workspace(n_rows, SENTINEL);
out_offsets.resize(n_rows + 1);
out_indices.resize(total_entries);
out_values.resize(total_entries);
i_t nz = 0;
for (i_t i = 0; i < n_rows; ++i) {
i_t row_start_out = nz;
out_offsets[i] = row_start_out;
for (i_t p = temp_offsets[i]; p < temp_offsets[i + 1]; ++p) {
i_t j = temp_indices[p];
f_t x = temp_values[p];
- if (workspace[j] >= row_start_out) {
+ if (workspace[j] != SENTINEL && workspace[j] >= row_start_out) {
out_values[workspace[j]] += x;
} else {
workspace[j] = nz;
out_indices[nz] = j;
out_values[nz] = x;
nz++;
}
}
}Alternatively, add a static_assert to enforce signed index types:
static_assert(std::is_signed<i_t>::value, "i_t must be a signed integer type");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/libmps_parser/src/utilities/sparse_matrix_utils.hpp` around lines 86 -
108, The workspace sentinel uses -1 which breaks when i_t is unsigned; add a
compile-time guard to require signed index types by inserting
static_assert(std::is_signed<i_t>::value, "i_t must be a signed integer type");
near the top of the function/template (before workspace is initialized) so
workspace, temp_offsets, temp_indices and related logic remain correct for all
builds; alternatively, replace the -1 sentinel with a sentinel value based on
std::numeric_limits<i_t>::min() and adjust the visited check accordingly if you
prefer a runtime-compatible sentinel instead of the static_assert.
Comment on lines
+1024
to
+1128
| std::string input_file = | ||
| cuopt::test::get_rapids_dataset_root_dir() + "/linear_programming/good-mps-1.mps"; | ||
| std::string temp_file = "/tmp/mps_roundtrip_lp_test.mps"; | ||
|
|
||
| // Read original | ||
| auto original = parse_mps<int, double>(input_file, true); | ||
|
|
||
| // Write to temp file | ||
| mps_writer_t<int, double> writer(original); | ||
| writer.write(temp_file); | ||
|
|
||
| // Read back | ||
| auto reloaded = parse_mps<int, double>(temp_file, false); | ||
|
|
||
| // Compare | ||
| compare_data_models(original, reloaded); | ||
|
|
||
| // Cleanup | ||
| std::filesystem::remove(temp_file); | ||
| } | ||
|
|
||
| TEST(mps_roundtrip, linear_programming_with_bounds) | ||
| { | ||
| if (!file_exists("linear_programming/lp_model_with_var_bounds.mps")) { | ||
| GTEST_SKIP() << "Test file not found"; | ||
| } | ||
|
|
||
| std::string input_file = | ||
| cuopt::test::get_rapids_dataset_root_dir() + "/linear_programming/lp_model_with_var_bounds.mps"; | ||
| std::string temp_file = "/tmp/mps_roundtrip_lp_bounds_test.mps"; | ||
|
|
||
| // Read original | ||
| auto original = parse_mps<int, double>(input_file, false); | ||
|
|
||
| // Write to temp file | ||
| mps_writer_t<int, double> writer(original); | ||
| writer.write(temp_file); | ||
|
|
||
| // Read back | ||
| auto reloaded = parse_mps<int, double>(temp_file, false); | ||
|
|
||
| // Compare | ||
| compare_data_models(original, reloaded); | ||
|
|
||
| // Cleanup | ||
| std::filesystem::remove(temp_file); | ||
| } | ||
|
|
||
| TEST(mps_roundtrip, quadratic_programming_qp_test_1) | ||
| { | ||
| if (!file_exists("quadratic_programming/QP_Test_1.qps")) { | ||
| GTEST_SKIP() << "Test file not found"; | ||
| } | ||
|
|
||
| std::string input_file = | ||
| cuopt::test::get_rapids_dataset_root_dir() + "/quadratic_programming/QP_Test_1.qps"; | ||
| std::string temp_file = "/tmp/mps_roundtrip_qp_test_1.mps"; | ||
|
|
||
| // Read original | ||
| auto original = parse_mps<int, double>(input_file, false); | ||
| ASSERT_TRUE(original.has_quadratic_objective()) << "Original should have quadratic objective"; | ||
|
|
||
| // Write to temp file | ||
| mps_writer_t<int, double> writer(original); | ||
| writer.write(temp_file); | ||
|
|
||
| // Read back | ||
| auto reloaded = parse_mps<int, double>(temp_file, false); | ||
| ASSERT_TRUE(reloaded.has_quadratic_objective()) << "Reloaded should have quadratic objective"; | ||
|
|
||
| // Compare | ||
| compare_data_models(original, reloaded); | ||
|
|
||
| // Cleanup | ||
| std::filesystem::remove(temp_file); | ||
| } | ||
|
|
||
| TEST(mps_roundtrip, quadratic_programming_qp_test_2) | ||
| { | ||
| if (!file_exists("quadratic_programming/QP_Test_2.qps")) { | ||
| GTEST_SKIP() << "Test file not found"; | ||
| } | ||
|
|
||
| std::string input_file = | ||
| cuopt::test::get_rapids_dataset_root_dir() + "/quadratic_programming/QP_Test_2.qps"; | ||
| std::string temp_file = "/tmp/mps_roundtrip_qp_test_2.mps"; | ||
|
|
||
| // Read original | ||
| auto original = parse_mps<int, double>(input_file, false); | ||
| ASSERT_TRUE(original.has_quadratic_objective()) << "Original should have quadratic objective"; | ||
|
|
||
| // Write to temp file | ||
| mps_writer_t<int, double> writer(original); | ||
| writer.write(temp_file); | ||
|
|
||
| // Read back | ||
| auto reloaded = parse_mps<int, double>(temp_file, false); | ||
| ASSERT_TRUE(reloaded.has_quadratic_objective()) << "Reloaded should have quadratic objective"; | ||
|
|
||
| // Compare | ||
| compare_data_models(original, reloaded); | ||
|
|
||
| // Cleanup | ||
| std::filesystem::remove(temp_file); | ||
| } |
There was a problem hiding this comment.
Use unique temp files + RAII cleanup for round-trip tests
Line 1026 and similar lines use fixed /tmp/... filenames. Under parallel gtest runs, this can cause collisions and flaky failures; also cleanup is skipped on early ASSERT_* returns.
🛠️ Proposed fix
+#include <chrono>
+#include <thread>
+
+namespace {
+struct temp_file_guard {
+ std::string path;
+ ~temp_file_guard()
+ {
+ std::error_code ec;
+ std::filesystem::remove(path, ec);
+ }
+};
+
+std::string make_temp_mps_path(const std::string& stem)
+{
+ std::stringstream ss;
+ ss << std::filesystem::temp_directory_path().string() << "/" << stem << "_"
+ << std::chrono::steady_clock::now().time_since_epoch().count() << "_"
+ << std::hash<std::thread::id>{}(std::this_thread::get_id()) << ".mps";
+ return ss.str();
+}
+} // namespace
...
- std::string temp_file = "/tmp/mps_roundtrip_lp_test.mps";
+ std::string temp_file = make_temp_mps_path("mps_roundtrip_lp_test");
+ temp_file_guard guard{temp_file};
...
- std::filesystem::remove(temp_file);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/libmps_parser/tests/mps_parser_test.cpp` around lines 1024 - 1128, The
tests (e.g., TEST mps_roundtrip::linear_programming,
linear_programming_with_bounds, quadratic_programming_qp_test_1/2) currently use
fixed temp_file paths like "/tmp/mps_roundtrip_*.mps" and call
std::filesystem::remove(...) manually, which can collide in parallel runs and
won't run if a test ASSERT_* aborts; change to generate unique temp paths (use
std::filesystem::temp_directory_path() + std::filesystem::unique_path() or
similar) and manage removal with an RAII file-cleanup helper (a small
scope-local class that holds the temp filename and deletes it in its
destructor); update usages where temp_file and writer.write(temp_file) occur and
remove the manual std::filesystem::remove(...) calls so cleanup always runs even
on early returns.
Comment on lines
+197
to
+205
| // Pass 1: Count entries per row in Q + Q^T | ||
| std::vector<i_t> row_counts(qn, 0); | ||
| for (i_t i = 0; i < qn; ++i) { | ||
| for (i_t p = Q_offsets[i]; p < Q_offsets[i + 1]; ++p) { | ||
| i_t j = Q_indices[p]; | ||
| row_counts[i]++; | ||
| if (i != j) { row_counts[j]++; } | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate Q_indices before indexing row buffers
Line 201 feeds j directly into row_counts[j], row_pos[j], and later workspace[j] (Line 255) with no bounds check. Malformed CSR input can trigger out-of-bounds writes.
🛡️ Proposed fix
+ cuopt_expects(size_indices == size_values,
+ error_type_t::ValidationError,
+ "Q_indices and Q_values must have the same size");
+ cuopt_expects(Q_offsets[0] == 0,
+ error_type_t::ValidationError,
+ "Q_offsets must start at 0");
+ cuopt_expects(Q_offsets[size_offsets - 1] == size_indices,
+ error_type_t::ValidationError,
+ "Q_offsets back() must equal size_indices");
...
for (i_t i = 0; i < qn; ++i) {
+ cuopt_expects(Q_offsets[i] <= Q_offsets[i + 1],
+ error_type_t::ValidationError,
+ "Q_offsets must be non-decreasing");
for (i_t p = Q_offsets[i]; p < Q_offsets[i + 1]; ++p) {
i_t j = Q_indices[p];
+ cuopt_expects(j >= 0 && j < qn,
+ error_type_t::ValidationError,
+ "Q_indices contains out-of-range column index");
row_counts[i]++;
if (i != j) { row_counts[j]++; }
}
}Also applies to: 222-236, 255-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/pdlp/optimization_problem.cu` around lines 197 - 205, The code
currently indexes Q_indices and uses each j to update row_counts[j], row_pos[j],
and workspace[j] without bounds checks; add validation that each j read from
Q_indices[p] is within [0, qn) before any write or increment and handle
violations by returning an error or failing early (e.g., validate inside the
loops in the Pass 1 and Pass 2 blocks that process Q_offsets/Q_indices and bail
with a clear error code/message if j < 0 || j >= qn); also validate Q_offsets
array shape (size == qn+1 and non-decreasing) before iterating to avoid
out-of-bounds reads when computing ranges used by functions that manipulate
row_counts, row_pos, and workspace.
Comment on lines
+207
to
+216
| // Build temporary offsets via prefix sum | ||
| std::vector<i_t> temp_offsets(qn + 1); | ||
| temp_offsets[0] = 0; | ||
| for (i_t i = 0; i < qn; ++i) { | ||
| temp_offsets[i + 1] = temp_offsets[i] + row_counts[i]; | ||
| } | ||
|
|
||
| i_t total_entries = temp_offsets[qn]; | ||
| std::vector<i_t> temp_indices(total_entries); | ||
| std::vector<f_t> temp_values(total_entries); |
There was a problem hiding this comment.
Protect CSR size accumulation from i_t overflow
temp_offsets and total_entries are computed in i_t. Large Q inputs can overflow and under-allocate temp_indices/temp_values, which is unsafe.
📏 Proposed fix
- std::vector<i_t> temp_offsets(qn + 1);
- temp_offsets[0] = 0;
+ std::vector<int64_t> temp_offsets64(qn + 1, 0);
for (i_t i = 0; i < qn; ++i) {
- temp_offsets[i + 1] = temp_offsets[i] + row_counts[i];
+ temp_offsets64[i + 1] = temp_offsets64[i] + static_cast<int64_t>(row_counts[i]);
}
-
- i_t total_entries = temp_offsets[qn];
+ cuopt_expects(temp_offsets64[qn] <= std::numeric_limits<i_t>::max(),
+ error_type_t::ValidationError,
+ "Quadratic objective too large");
+ i_t total_entries = static_cast<i_t>(temp_offsets64[qn]);
+ std::vector<i_t> temp_offsets(qn + 1);
+ for (i_t i = 0; i <= qn; ++i) { temp_offsets[i] = static_cast<i_t>(temp_offsets64[i]); }Also applies to: 240-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/pdlp/optimization_problem.cu` around lines 207 - 216, The CSR size
accumulation currently uses i_t (temp_offsets, total_entries) which can overflow
for large qn; change the accumulation and capacity checks to use a wider
unsigned/signed type (e.g., size_t or int64_t) when computing temp_offsets and
total_entries, validate that total_entries fits in size_t and is non-negative
before allocating temp_indices/temp_values, and add an explicit overflow check
when summing row_counts (e.g., detect if temp_offsets[i]+row_counts[i] <
temp_offsets[i] or exceeds a max allowed size) to early-return/error; apply the
same change to the other accumulation block handling temp_offsets/total_entries
around the later code paths that build CSR (functions/variables: temp_offsets,
total_entries, temp_indices, temp_values, row_counts, qn, i_t).
| std::vector<i_t>& out_indices, | ||
| std::vector<i_t>& out_offsets) | ||
| { | ||
| if (in_offsets.size() <= 1) { return; } |
There was a problem hiding this comment.
Early return leaves output vectors in undefined state.
When in_offsets.size() <= 1, the function returns early without initializing the output vectors. Callers expecting a valid empty CSR representation (e.g., out_offsets = {0}) will get whatever state the vectors had before the call.
🛡️ Proposed fix to initialize outputs on early return
if (in_offsets.size() <= 1) {
+ out_values.clear();
+ out_indices.clear();
+ out_offsets.assign(1, 0); // Valid empty CSR: single offset of 0
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/utilities/sparse_matrix_helpers.hpp` at line 144, The early return
when in_offsets.size() <= 1 leaves output vectors uninitialized; update the
function that takes in_offsets and writes to out_offsets, out_col_indices and
out_values so that before returning it sets out_offsets to a valid empty CSR
(e.g., out_offsets = {0}) and clears out_col_indices and out_values (or
otherwise initializes them to empty consistent CSR state), then return; modify
the branch around the check of in_offsets.size() to perform these assignments to
ensure callers receive a well-defined empty matrix.
Contributor
Author
|
/ok to test 0f3ca8e |
chris-maes
reviewed
Mar 20, 2026
| f_t norm_b = vector_norm_inf<i_t, f_t>(data.b, stream_view_); | ||
| f_t norm_c = vector_norm_inf<i_t, f_t>(data.c, stream_view_); | ||
|
|
||
| settings.log.printf("norm_b: %.2e, norm_c: %.2e\n", norm_b, norm_c); |
chris-maes
reviewed
Mar 20, 2026
| * @param[out] out_offsets Output CSR row offsets | ||
| */ | ||
| template <typename i_t, typename f_t> | ||
| void symmetrize_csr(const f_t* in_values, |
Contributor
There was a problem hiding this comment.
Is this function repeated in both sparse_matrix_utils and sparse_matrix_helpers?
chris-maes
requested changes
Mar 20, 2026
Contributor
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
It looks like some functions are duplicated. Also would you mind removing the debug print from barrier before merging?
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
3 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.
Description
This PR adds writing of quadratic terms to mps file in the mps writer.
Issue
Checklist