Write quadratic terms to mps file #949
Open
rg20 wants to merge 11 commits intoNVIDIA:release/26.04from
Open
Conversation
rg20
added
bug
📝 WalkthroughWalkthroughThe changes introduce MPS writer functionality for both linear and quadratic programming problems. A new CSR matrix symmetrization utility is added to support quadratic objective handling without intermediate representations. Integration updates across optimization modules delegate symmetrization to this utility. Presolve improves numerical stability with compensated summation, and auxiliary data storage clarifications are added to CPU optimization code. 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)
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: 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
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
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.
cpp/src/pdlp/optimization_problem.cu
Outdated
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.
cpp/src/pdlp/optimization_problem.cu
Outdated
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
cpp/src/barrier/barrier.cu
Outdated
| 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?
Contributor
Author
|
/ok to test f6a8240 |
Contributor
Author
Thanks for catching. There were too many conflicts with remote execution PR and that resulted in some duplications. Fixed them now. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cpp/src/utilities/sparse_matrix_helpers.hpp (2)
148-148:⚠️ Potential issue | 🟠 MajorReturn a valid empty CSR on the fast path.
Line 148 exits without touching the outputs. If the caller reuses non-empty buffers, the previous matrix survives and the next consumer sees stale quadratic terms.
🛠️ Proposed fix
- if (in_offsets.size() <= 1) { return; } + if (in_offsets.size() <= 1) { + out_values.clear(); + out_indices.clear(); + out_offsets.assign(1, 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 148, The early exit when in_offsets.size() <= 1 leaves output buffers untouched; change the fast path to write a valid empty CSR instead of returning: set the output offsets to a single zero entry (e.g., out_offsets.resize(1); out_offsets[0]=0), clear/resize out_indices and out_values to empty, and then return. Update the code path that currently checks in_offsets to use these symbols (in_offsets, out_offsets, out_indices, out_values) so callers never see stale data.
52-69:⚠️ Potential issue | 🟠 MajorUse a wider type for the count/prefix-sum before sizing temp storage.
row_counts,temp_offsets, andtotal_entriesare alli_t. Large quadratic matrices can wrap these accumulators, under-allocatetemp_indices/temp_values, and then overrun them in Pass 2/3. Count inint64_t/size_t, verify the final nnz fits ini_t, then downcast.As per coding guidelines, "Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems."
🤖 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 52 - 69, The prefix-sum/count accumulators (row_counts, temp_offsets, and total_entries) use the narrow type i_t which can overflow for large problems; change the counting and prefix-sum to a wider integer (e.g., int64_t or size_t) when computing row_counts and temp_offsets and for total_entries, validate that total_entries fits in i_t before allocating temp_indices/temp_values, then downcast to i_t for storage; update the code locations referencing row_counts, temp_offsets, total_entries, temp_indices, and any subsequent passes that assume i_t to use the safe checked value.cpp/src/pdlp/optimization_problem.cu (1)
193-197:⚠️ Potential issue | 🔴 CriticalValidate and stage the quadratic CSR before calling the host helper.
cuopt::symmetrize_csriteratesQ_offsets/Q_indices/Q_valueson the CPU. This path now only checks null/non-empty, so a size mismatch (size_values != size_indices, terminal offset not matching them), non-monotone offsets, out-of-rangeQ_indices, or a device-resident buffer will turn into invalid host memory access instead of a validation error. Please restore the CSR shape/range checks and copy the inputs to host before symmetrizing.As per coding guidelines, "Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems."
🤖 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 193 - 197, Restore full CSR validation for Q before calling cuopt::symmetrize_csr: verify size_offsets >= 1 and terminal offset equals size_indices, check size_values == size_indices, ensure offsets are non-decreasing and within [0, size_indices], and that all Q_indices entries are in [0, qn); if any check fails return a clear validation error. After validation, ensure Q_values/Q_indices/Q_offsets are copied to host memory (if currently device buffers) into host arrays (e.g., Q_values_, Q_indices_, Q_offsets_) and only then call cuopt::symmetrize_csr<i_t,f_t>(Q_values_, Q_indices_, Q_offsets_, qn, ...). Use the existing symbols Q_values, Q_indices, Q_offsets, size_values, size_indices, size_offsets, qn, and Q_values_/Q_indices_/Q_offsets_ to locate and implement these checks and the host staging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cpp/src/pdlp/optimization_problem.cu`:
- Around line 193-197: Restore full CSR validation for Q before calling
cuopt::symmetrize_csr: verify size_offsets >= 1 and terminal offset equals
size_indices, check size_values == size_indices, ensure offsets are
non-decreasing and within [0, size_indices], and that all Q_indices entries are
in [0, qn); if any check fails return a clear validation error. After
validation, ensure Q_values/Q_indices/Q_offsets are copied to host memory (if
currently device buffers) into host arrays (e.g., Q_values_, Q_indices_,
Q_offsets_) and only then call cuopt::symmetrize_csr<i_t,f_t>(Q_values_,
Q_indices_, Q_offsets_, qn, ...). Use the existing symbols Q_values, Q_indices,
Q_offsets, size_values, size_indices, size_offsets, qn, and
Q_values_/Q_indices_/Q_offsets_ to locate and implement these checks and the
host staging.
In `@cpp/src/utilities/sparse_matrix_helpers.hpp`:
- Line 148: The early exit when in_offsets.size() <= 1 leaves output buffers
untouched; change the fast path to write a valid empty CSR instead of returning:
set the output offsets to a single zero entry (e.g., out_offsets.resize(1);
out_offsets[0]=0), clear/resize out_indices and out_values to empty, and then
return. Update the code path that currently checks in_offsets to use these
symbols (in_offsets, out_offsets, out_indices, out_values) so callers never see
stale data.
- Around line 52-69: The prefix-sum/count accumulators (row_counts,
temp_offsets, and total_entries) use the narrow type i_t which can overflow for
large problems; change the counting and prefix-sum to a wider integer (e.g.,
int64_t or size_t) when computing row_counts and temp_offsets and for
total_entries, validate that total_entries fits in i_t before allocating
temp_indices/temp_values, then downcast to i_t for storage; update the code
locations referencing row_counts, temp_offsets, total_entries, temp_indices, and
any subsequent passes that assume i_t to use the safe checked value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d94dff99-6151-473d-b329-8f953663ae87
📒 Files selected for processing (5)
cpp/libmps_parser/CMakeLists.txtcpp/libmps_parser/src/mps_writer.cppcpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/optimization_problem.cucpp/src/utilities/sparse_matrix_helpers.hpp
✅ Files skipped from review due to trivial changes (1)
- cpp/src/pdlp/cpu_optimization_problem.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/libmps_parser/src/mps_writer.cpp
chris-maes
added
improvement
chris-maes
approved these changes
Mar 25, 2026
Contributor
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
LGTM. You might add to the commit message that the PR includes the Kahan summation in presolve when fixing columns.
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