Open
Conversation
hlinsen
added
feature request
📝 WalkthroughWalkthroughThis pull request implements support for semi-continuous variables in the MIP solver. Semi-continuous variables satisfy the constraint X = 0 or L ≤ X ≤ U. The implementation includes MPS parser enhancements, variable type definitions, reformulation logic using binary variables and constraints, validation checks, and comprehensive test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
cpp/CMakeLists.txt (1)
292-297: Align OpenSSL discovery with gRPC/protobuf patterns for consistency.The current fallback logic checks
OpenSSL_FOUND/OPENSSL_FOUNDvariables before retrying discovery, but the identical patterns for gRPC and protobuf (lines 303–310) rely solely on target-based checks. This inconsistency is unnecessary—since FOUND variables can be unreliable for confirming target availability, use target re-checks instead.Suggested patch
if(NOT TARGET OpenSSL::SSL) find_package(OpenSSL CONFIG QUIET) - if(NOT OpenSSL_FOUND AND NOT OPENSSL_FOUND) - find_package(OpenSSL REQUIRED) - endif() +endif() +if(NOT TARGET OpenSSL::SSL) + find_package(OpenSSL REQUIRED) endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/CMakeLists.txt` around lines 292 - 297, The OpenSSL discovery block should mirror the gRPC/protobuf pattern by relying on target checks instead of FOUND variables; replace the current conditional that tests OpenSSL_FOUND/OPENSSL_FOUND with a second check for the OpenSSL::SSL target. Concretely, keep the initial if(NOT TARGET OpenSSL::SSL) find_package(OpenSSL CONFIG QUIET) call and then, instead of testing OpenSSL_FOUND or OPENSSL_FOUND, re-check if(NOT TARGET OpenSSL::SSL) and only then call find_package(OpenSSL REQUIRED) so discovery consistently uses target-based checks.cpp/include/cuopt/linear_programming/mip/solver_settings.hpp (1)
89-89: Appropriate default for Big-M parameter.The default value of
1e5forsc_big_mis a reasonable choice for Big-M reformulations. Users can adjust this if their problem has larger variable bounds.Consider adding a brief inline comment explaining this parameter's purpose, similar to other settings in this file (e.g.,
node_limit,max_cut_passes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/include/cuopt/linear_programming/mip/solver_settings.hpp` at line 89, Add a brief inline comment explaining the purpose and guidance for the Big‑M parameter by annotating the sc_big_m setting: clarify that sc_big_m (default f_t(1e5)) is the Big‑M constant used in reformulations, why 1e5 is a reasonable default and that users may increase it for problems with larger variable bounds; place the comment adjacent to the sc_big_m declaration so it matches style of other settings like node_limit and max_cut_passes.cpp/tests/mip/miplib_test.cu (1)
111-136: Add at least one regression where the SC variable is active.Both new MPS cases optimize to
x = 0, so they only cover the inactive branch. Theb = 1linking constraints, positive-interval postsolve path, and fallback-sc_big_mhandling are still untested here.As per coding guidelines, "Add tests for problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/tests/mip/miplib_test.cu` around lines 111 - 136, The current test vector in mip_solve::semi_continuous_regressions only includes cases where the semi-continuous (SC) variable is inactive; add at least one MPS test instance where the SC variable is active (e.g., "mip/sc_active.mps") and include the expected objective and non-zero sc_value in the test_instances initializer (sc_result_t) so the loop exercises the active-branch, the b=1 linking constraint, the positive-interval postsolve, and fallback sc_big_m handling; update/verify expectations used by parse_mps, solve_mip, and host_copy in this test so EXPECT_EQ/EXPECT_NEAR assertions validate the original→transformed→postsolve mappings and index consistency for that active instance.
🤖 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/include/cuopt/linear_programming/optimization_problem_interface.hpp`:
- Line 23: The new enum value var_t::SEMI_CONTINUOUS was added but the GRPC
mapping logic still only handles CONTINUOUS/INTEGER and throws in the default
branch; update the mappings in cpp/src/grpc/grpc_problem_mapper.cpp to handle
SEMI_CONTINUOUS everywhere it is converted. Specifically, add cases for
var_t::SEMI_CONTINUOUS in the var_t -> proto enum mapper, the proto enum ->
var_t mapper, and the chunked-transfer/remote-solve mapper that currently throws
on unknown values so that SEMI_CONTINUOUS is correctly translated in all three
places (preserve existing switch/return patterns and cover the default branch
accordingly).
In `@cpp/include/cuopt/linear_programming/optimization_problem_utils.hpp`:
- Around line 261-263: The MPS writer currently treats semi-continuous variables
like continuous ones, causing loss of 'S' on round-trips; update the
bounds-serialization logic in the MPS writer (mps_writer) to check
variable_types[j] == 'S' and emit the 'SC' bound marker (and corresponding bound
lines) for those variables just like the integer branch emits "LI"/"UI"—i.e.,
add a branch alongside the existing integer ('I'/'B') handling so
semi-continuous variables are serialized with 'SC' and preserved when parsing
back (refer to variable_types[j] and the bounds-selection code in the writer).
In `@cpp/libmps_parser/tests/mps_parser_test.cpp`:
- Line 19: Remove the unused include directive for <fstream> in the test file:
delete the line containing `#include` <fstream> (the header added at top of
cpp/libmps_parser/tests/mps_parser_test.cpp) since file operations use
std::filesystem and no symbol from <fstream> is referenced.
In `@cpp/src/mip_heuristics/presolve/sc_reformulation.cu`:
- Around line 147-151: The code assigns big_m into U without validating it;
change the fallback logic in the block handling non-finite U so that before
assigning U = big_m you verify std::isfinite(big_m) && (big_m > L) (where L is
the corresponding lower bound used earlier or orig_u), and only then set U and
mark used_fallback_big_m[idx]=1; otherwise keep U as orig_u (or leave it
non-finite and handle as before) and do not mark the fallback used. Update
references around U, orig_u, big_m, used_fallback_big_m and idx to ensure the
fallback flag is only set when the validated big_m was actually applied.
In `@cpp/src/mip_heuristics/solve.cu`:
- Around line 291-304: The in-place semi-continuous reformulation
(detail::reformulate_semi_continuous) mutates op_problem and adds auxiliary
binaries so op_problem.get_n_variables() no longer equals the user's original
variable count; ensure all MIP callback setup and payloads (GET_SOLUTION /
SET_SOLUTION code paths) use the saved n_orig_before_sc rather than the
reformulated dimension, and trim any solution vectors or callback assignments to
the original prefix of length n_orig_before_sc before invoking user callbacks;
update any callback registration or solution-copy logic to reference
n_orig_before_sc and strip trailing auxiliary variables added by the
reformulation prior to calling user code.
- Around line 291-298: Move creation/starting of the solve timer (the timer_t
instance) to before the semi‑continuous reformulation so the SC phase is counted
against settings.time_limit; specifically, create/start the timer prior to
calling detail::reformulate_semi_continuous(op_problem, settings,
&sc_used_fallback_big_m) (and pass the timer into that routine if it accepts
one) so the time spent in sc_reformulation.cu is budgeted and included in the
reported presolve/solve times; ensure any later references to timer_t (e.g.,
when recording presolve time) still use this same instance so reported timings
include the SC work.
In `@cpp/src/pdlp/utilities/problem_checking.cu`:
- Around line 220-251: The loop in problem_checking.cu indexes var_lb and var_ub
without ensuring bounds were materialized, which can crash when callers set
SEMI_CONTINUOUS types without initializing bounds; before calling
op_problem.get_variable_lower_bounds_host() / get_variable_upper_bounds_host()
or before indexing var_lb[i]/var_ub[i], add a guard that verifies the bounds
vectors are present and sized to match var_types (e.g., check that
op_problem.get_variable_lower_bounds().is_empty() is false or that var_lb.size()
== var_types.size()), and if not, emit a cuopt_expects ValidationError with a
clear message about missing variable bounds for SEMI_CONTINUOUS variables; keep
the existing per-variable SEMI_CONTINUOUS checks (var_types, var_lb, var_ub,
cuopt_expects) but only after this presence/size validation.
---
Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Around line 292-297: The OpenSSL discovery block should mirror the
gRPC/protobuf pattern by relying on target checks instead of FOUND variables;
replace the current conditional that tests OpenSSL_FOUND/OPENSSL_FOUND with a
second check for the OpenSSL::SSL target. Concretely, keep the initial if(NOT
TARGET OpenSSL::SSL) find_package(OpenSSL CONFIG QUIET) call and then, instead
of testing OpenSSL_FOUND or OPENSSL_FOUND, re-check if(NOT TARGET OpenSSL::SSL)
and only then call find_package(OpenSSL REQUIRED) so discovery consistently uses
target-based checks.
In `@cpp/include/cuopt/linear_programming/mip/solver_settings.hpp`:
- Line 89: Add a brief inline comment explaining the purpose and guidance for
the Big‑M parameter by annotating the sc_big_m setting: clarify that sc_big_m
(default f_t(1e5)) is the Big‑M constant used in reformulations, why 1e5 is a
reasonable default and that users may increase it for problems with larger
variable bounds; place the comment adjacent to the sc_big_m declaration so it
matches style of other settings like node_limit and max_cut_passes.
In `@cpp/tests/mip/miplib_test.cu`:
- Around line 111-136: The current test vector in
mip_solve::semi_continuous_regressions only includes cases where the
semi-continuous (SC) variable is inactive; add at least one MPS test instance
where the SC variable is active (e.g., "mip/sc_active.mps") and include the
expected objective and non-zero sc_value in the test_instances initializer
(sc_result_t) so the loop exercises the active-branch, the b=1 linking
constraint, the positive-interval postsolve, and fallback sc_big_m handling;
update/verify expectations used by parse_mps, solve_mip, and host_copy in this
test so EXPECT_EQ/EXPECT_NEAR assertions validate the
original→transformed→postsolve mappings and index consistency for that active
instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1383efd9-9c0c-427d-a6d6-8e526702b927
📒 Files selected for processing (21)
cpp/CMakeLists.txtcpp/cuopt_cli.cppcpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/optimization_problem_interface.hppcpp/include/cuopt/linear_programming/optimization_problem_utils.hppcpp/libmps_parser/src/mps_parser.cppcpp/libmps_parser/src/mps_parser.hppcpp/libmps_parser/tests/mps_parser_test.cppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/CMakeLists.txtcpp/src/mip_heuristics/presolve/sc_reformulation.cucpp/src/mip_heuristics/presolve/sc_reformulation.cuhcpp/src/mip_heuristics/solve.cucpp/src/pdlp/optimization_problem.cucpp/src/pdlp/solve.cucpp/src/pdlp/utilities/problem_checking.cucpp/tests/mip/miplib_test.cudatasets/mip/sc_lb_zero.mpsdatasets/mip/sc_no_ub.mpsdatasets/mip/sc_standard.mps
💤 Files with no reviewable changes (1)
- cpp/cuopt_cli.cpp
| namespace cuopt::linear_programming { | ||
|
|
||
| enum class var_t { CONTINUOUS = 0, INTEGER }; | ||
| enum class var_t { CONTINUOUS = 0, INTEGER, SEMI_CONTINUOUS }; |
There was a problem hiding this comment.
Update the remote/grpc enum mappers in the same change.
Line 23 adds var_t::SEMI_CONTINUOUS, but cpp/src/grpc/grpc_problem_mapper.cpp:108-119, cpp/src/grpc/grpc_problem_mapper.cpp:131-141, and cpp/src/grpc/grpc_problem_mapper.cpp:638-648 still only map CONTINUOUS/INTEGER and throw in the default branch. Any remote solve or chunked transfer that carries an SC variable will still fail at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/include/cuopt/linear_programming/optimization_problem_interface.hpp` at
line 23, The new enum value var_t::SEMI_CONTINUOUS was added but the GRPC
mapping logic still only handles CONTINUOUS/INTEGER and throws in the default
branch; update the mappings in cpp/src/grpc/grpc_problem_mapper.cpp to handle
SEMI_CONTINUOUS everywhere it is converted. Specifically, add cases for
var_t::SEMI_CONTINUOUS in the var_t -> proto enum mapper, the proto enum ->
var_t mapper, and the chunked-transfer/remote-solve mapper that currently throws
on unknown values so that SEMI_CONTINUOUS is correctly translated in all three
places (preserve existing switch/return patterns and cover the default branch
accordingly).
Comment on lines
+261
to
+263
| if (val == 'I' || val == 'B') return var_t::INTEGER; | ||
| if (val == 'S') return var_t::SEMI_CONTINUOUS; | ||
| return var_t::CONTINUOUS; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MPS writer handles 'S' type variables for semi-continuous bounds
rg -n "'S'" cpp/libmps_parser/src/mps_writer.cpp
# Check how variable types are handled in the writer
rg -n -C3 "variable_types" cpp/libmps_parser/src/mps_writer.cpp | head -60Repository: NVIDIA/cuopt
Length of output: 3159
🏁 Script executed:
#!/bin/bash
# Verify the exact file and lines mentioned in the review
wc -l cpp/include/cuopt/linear_programming/optimization_problem_utils.hpp
sed -n '255,270p' cpp/include/cuopt/linear_programming/optimization_problem_utils.hpp
# Check for any 'SC' bounds handling in MPS writer
rg -n "'SC'" cpp/libmps_parser/src/mps_writer.cpp
# Check for semi-continuous handling anywhere in the writer
rg -n -i "semi" cpp/libmps_parser/src/mps_writer.cpp
# Get full context of bounds writing section
sed -n '335,370p' cpp/libmps_parser/src/mps_writer.cppRepository: NVIDIA/cuopt
Length of output: 2640
🏁 Script executed:
#!/bin/bash
# Search for any comments about semi-continuous in MPS writer
rg -n -i "semi\|sc bounds\|bound.*type" cpp/libmps_parser/src/mps_writer.cpp | head -20
# Check the MPS data model to understand how SC is meant to be handled
rg -n "'S'" cpp/libmps_parser/src/ -A2 -B2 | head -40
# Look at comments/docs about MPS writer scope
head -50 cpp/libmps_parser/src/mps_writer.cppRepository: NVIDIA/cuopt
Length of output: 1905
Code at lines 261-263 is correct; MPS writer lacks semi-continuous serialization.
The lambda correctly converts the 'S' character to var_t::SEMI_CONTINUOUS, maintaining consistency with the parser's handling and mirroring populate_from_mps_data_model logic. However, the MPS writer does not serialize semi-continuous variables: it only distinguishes integer variables ('I') for bound type selection ("LI"/"UI" vs "LO"/"UP"), treating semi-continuous variables identically to continuous ones. This creates an inconsistency where variables with type 'S' parse correctly but lose their semi-continuous classification on round-trip through the writer. The MPS format supports the 'SC' bound marker for semi-continuous bounds, which should be implemented in cpp/libmps_parser/src/mps_writer.cpp to properly serialize variables with variable_types[j] == 'S'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/include/cuopt/linear_programming/optimization_problem_utils.hpp` around
lines 261 - 263, The MPS writer currently treats semi-continuous variables like
continuous ones, causing loss of 'S' on round-trips; update the
bounds-serialization logic in the MPS writer (mps_writer) to check
variable_types[j] == 'S' and emit the 'SC' bound marker (and corresponding bound
lines) for those variables just like the integer branch emits "LI"/"UI"—i.e.,
add a branch alongside the existing integer ('I'/'B') handling so
semi-continuous variables are serialized with 'SC' and preserved when parsing
back (refer to variable_types[j] and the bounds-selection code in the writer).
| #include <cmath> | ||
| #include <cstdint> | ||
| #include <filesystem> | ||
| #include <fstream> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if fstream types/functions are used in the test file
rg -n "ifstream|ofstream|fstream\b" cpp/libmps_parser/tests/mps_parser_test.cppRepository: NVIDIA/cuopt
Length of output: 77
Remove unused <fstream> include at line 19.
The <fstream> header was added but is never used in the test file; all file operations are performed via std::filesystem. This include should be removed to keep dependencies minimal.
🤖 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` at line 19, Remove the unused
include directive for <fstream> in the test file: delete the line containing
`#include` <fstream> (the header added at top of
cpp/libmps_parser/tests/mps_parser_test.cpp) since file operations use
std::filesystem and no symbol from <fstream> is referenced.
Comment on lines
+147
to
+151
| if (!std::isfinite(U)) { U = orig_u; } | ||
| if (!std::isfinite(U)) { | ||
| U = big_m; | ||
| if (used_fallback_big_m != nullptr) { (*used_fallback_big_m)[idx] = uint8_t{1}; } | ||
| } |
There was a problem hiding this comment.
Validate fallback sc_big_m before using it as U.
When propagation cannot derive a finite bound, this path injects settings.sc_big_m directly into the reformulation. If that value is non-finite or <= L, the new constraint gets an invalid or too-tight U, which can silently make the b = 1 branch impossible or return a truncated-model result without ever tripping the post-solve x ≈ M check on Line 580 of cpp/src/mip_heuristics/solve.cu.
Proposed validation
+#include <cuopt/error.hpp>
...
if (!std::isfinite(U)) {
+ cuopt_expects(std::isfinite(big_m) && big_m > L,
+ cuopt::error_type_t::ValidationError,
+ "sc_big_m must be finite and strictly greater than the semi-continuous lower bound when used as a fallback upper bound");
U = big_m;
if (used_fallback_big_m != nullptr) { (*used_fallback_big_m)[idx] = uint8_t{1}; }
}📝 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
| if (!std::isfinite(U)) { U = orig_u; } | |
| if (!std::isfinite(U)) { | |
| U = big_m; | |
| if (used_fallback_big_m != nullptr) { (*used_fallback_big_m)[idx] = uint8_t{1}; } | |
| } | |
| if (!std::isfinite(U)) { U = orig_u; } | |
| if (!std::isfinite(U)) { | |
| cuopt_expects(std::isfinite(big_m) && big_m > L, | |
| cuopt::error_type_t::ValidationError, | |
| "sc_big_m must be finite and strictly greater than the semi-continuous lower bound when used as a fallback upper bound"); | |
| U = big_m; | |
| if (used_fallback_big_m != nullptr) { (*used_fallback_big_m)[idx] = uint8_t{1}; } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/presolve/sc_reformulation.cu` around lines 147 - 151,
The code assigns big_m into U without validating it; change the fallback logic
in the block handling non-finite U so that before assigning U = big_m you verify
std::isfinite(big_m) && (big_m > L) (where L is the corresponding lower bound
used earlier or orig_u), and only then set U and mark
used_fallback_big_m[idx]=1; otherwise keep U as orig_u (or leave it non-finite
and handle as before) and do not mark the fallback used. Update references
around U, orig_u, big_m, used_fallback_big_m and idx to ensure the fallback flag
is only set when the validated big_m was actually applied.
Comment on lines
+291
to
+304
| // Reformulate semi-continuous variables (x = 0 OR L <= x <= U) before Papilo presolve. | ||
| // Uses GPU bounds propagation to derive tight upper bounds for SC vars with infinite UB. | ||
| // Track n_orig so that auxiliary binary variables added by reformulation can be stripped | ||
| // from the solution before returning it to the caller. | ||
| const i_t n_orig_before_sc = op_problem.get_n_variables(); | ||
| std::vector<uint8_t> sc_used_fallback_big_m; | ||
| const bool had_sc = | ||
| detail::reformulate_semi_continuous(op_problem, settings, &sc_used_fallback_big_m); | ||
| if (had_sc && !settings.initial_solutions.empty()) { | ||
| CUOPT_LOG_WARN( | ||
| "Ignoring %zu user initial solution(s): semi-continuous warm starts are not supported yet", | ||
| settings.initial_solutions.size()); | ||
| settings.initial_solutions.clear(); | ||
| } |
There was a problem hiding this comment.
Keep MIP callbacks in the original variable space.
After this in-place reformulation, op_problem.get_n_variables() includes the auxiliary SC binaries. The final return path trims them on Lines 566-570, but callback setup and callback payloads still use the reformulated dimension, so GET_SOLUTION / SET_SOLUTION clients will now see extra trailing variables that do not exist in the user model. Please use n_orig_before_sc for callback setup and trim callback assignments back to the original prefix before invoking user code.
As per coding guidelines, "Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/solve.cu` around lines 291 - 304, The in-place
semi-continuous reformulation (detail::reformulate_semi_continuous) mutates
op_problem and adds auxiliary binaries so op_problem.get_n_variables() no longer
equals the user's original variable count; ensure all MIP callback setup and
payloads (GET_SOLUTION / SET_SOLUTION code paths) use the saved n_orig_before_sc
rather than the reformulated dimension, and trim any solution vectors or
callback assignments to the original prefix of length n_orig_before_sc before
invoking user callbacks; update any callback registration or solution-copy logic
to reference n_orig_before_sc and strip trailing auxiliary variables added by
the reformulation prior to calling user code.
Comment on lines
+291
to
+298
| // Reformulate semi-continuous variables (x = 0 OR L <= x <= U) before Papilo presolve. | ||
| // Uses GPU bounds propagation to derive tight upper bounds for SC vars with infinite UB. | ||
| // Track n_orig so that auxiliary binary variables added by reformulation can be stripped | ||
| // from the solution before returning it to the caller. | ||
| const i_t n_orig_before_sc = op_problem.get_n_variables(); | ||
| std::vector<uint8_t> sc_used_fallback_big_m; | ||
| const bool had_sc = | ||
| detail::reformulate_semi_continuous(op_problem, settings, &sc_used_fallback_big_m); |
There was a problem hiding this comment.
Start the solve timer before SC reformulation.
timer_t is only created on Line 325, but this new phase can already spend up to five seconds in cpp/src/mip_heuristics/presolve/sc_reformulation.cu (bp_settings.time_limit = 5.0). A small settings.time_limit can now be exceeded before the solver starts, and the reported presolve time will miss that work. Please budget the SC reformulation against the user time limit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/solve.cu` around lines 291 - 298, Move
creation/starting of the solve timer (the timer_t instance) to before the
semi‑continuous reformulation so the SC phase is counted against
settings.time_limit; specifically, create/start the timer prior to calling
detail::reformulate_semi_continuous(op_problem, settings,
&sc_used_fallback_big_m) (and pass the timer into that routine if it accepts
one) so the time spent in sc_reformulation.cu is budgeted and included in the
reported presolve/solve times; ensure any later references to timer_t (e.g.,
when recording presolve time) still use this same instance so reported timings
include the SC work.
Comment on lines
+220
to
+251
| if (!op_problem.get_variable_types().is_empty()) { | ||
| cuopt_expects( | ||
| op_problem.get_variable_types().size() == op_problem.get_objective_coefficients().size(), | ||
| error_type_t::ValidationError, | ||
| "Sizes for vectors related to the variables are not the same. The objective " | ||
| "vector has size %zu and the variable types vector has size %zu.", | ||
| op_problem.get_objective_coefficients().size(), | ||
| op_problem.get_variable_types().size()); | ||
|
|
||
| auto var_types = op_problem.get_variable_types_host(); | ||
| auto var_lb = op_problem.get_variable_lower_bounds_host(); | ||
| auto var_ub = op_problem.get_variable_upper_bounds_host(); | ||
| for (i_t i = 0; i < static_cast<i_t>(var_types.size()); ++i) { | ||
| if (var_types[i] != var_t::SEMI_CONTINUOUS) { continue; } | ||
| cuopt_expects(var_lb[i] > f_t(0), | ||
| error_type_t::ValidationError, | ||
| "Semi-continuous variable must have a strictly positive lower bound, but has " | ||
| "lower bound %g.", | ||
| static_cast<double>(var_lb[i])); | ||
| cuopt_expects(var_ub[i] > f_t(0), | ||
| error_type_t::ValidationError, | ||
| "Semi-continuous variable must have a strictly positive upper bound, but has " | ||
| "upper bound %g.", | ||
| static_cast<double>(var_ub[i])); | ||
| cuopt_expects(var_lb[i] < var_ub[i], | ||
| error_type_t::ValidationError, | ||
| "Semi-continuous variable must satisfy lower bound < upper bound, but has " | ||
| "bounds [%g, %g].", | ||
| static_cast<double>(var_lb[i]), | ||
| static_cast<double>(var_ub[i])); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate SC bounds are present before indexing the host copies.
Direct API callers can set SEMI_CONTINUOUS types without materializing bounds first. In that case, Lines 234-249 read var_lb[i] and var_ub[i] before a clean validation error is emitted.
Suggested guard
if (!op_problem.get_variable_types().is_empty()) {
cuopt_expects(
op_problem.get_variable_types().size() == op_problem.get_objective_coefficients().size(),
error_type_t::ValidationError,
"Sizes for vectors related to the variables are not the same. The objective "
"vector has size %zu and the variable types vector has size %zu.",
op_problem.get_objective_coefficients().size(),
op_problem.get_variable_types().size());
auto var_types = op_problem.get_variable_types_host();
+ bool has_semi_continuous = false;
+ for (const auto type : var_types) {
+ if (type == var_t::SEMI_CONTINUOUS) {
+ has_semi_continuous = true;
+ break;
+ }
+ }
+ if (has_semi_continuous) {
+ cuopt_expects(op_problem.get_variable_lower_bounds().size() == var_types.size() &&
+ op_problem.get_variable_upper_bounds().size() == var_types.size(),
+ error_type_t::ValidationError,
+ "Semi-continuous variables require explicit lower and upper bounds.");
+ }
auto var_types = op_problem.get_variable_types_host();
auto var_lb = op_problem.get_variable_lower_bounds_host();
auto var_ub = op_problem.get_variable_upper_bounds_host();As per coding guidelines "Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/pdlp/utilities/problem_checking.cu` around lines 220 - 251, The loop
in problem_checking.cu indexes var_lb and var_ub without ensuring bounds were
materialized, which can crash when callers set SEMI_CONTINUOUS types without
initializing bounds; before calling op_problem.get_variable_lower_bounds_host()
/ get_variable_upper_bounds_host() or before indexing var_lb[i]/var_ub[i], add a
guard that verifies the bounds vectors are present and sized to match var_types
(e.g., check that op_problem.get_variable_lower_bounds().is_empty() is false or
that var_lb.size() == var_types.size()), and if not, emit a cuopt_expects
ValidationError with a clear message about missing variable bounds for
SEMI_CONTINUOUS variables; keep the existing per-variable SEMI_CONTINUOUS checks
(var_types, var_lb, var_ub, cuopt_expects) but only after this presence/size
validation.
2a5da0c to
4ca42b1
Compare
hlinsen
requested review from
chris-maes and
mlubin
and removed request for
Bubullzz and
nguidotti
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cpp/src/mip_heuristics/solve.cu (1)
318-321:⚠️ Potential issue | 🟠 MajorKeep callbacks in the original variable space.
n_orig_before_scis saved here, but callback setup and callback payloads still use the reformulated dimension later in this file and inrun_mip().GET_SOLUTION/SET_SOLUTIONclients will still see the auxiliary SC binaries unless those paths are trimmed back to the original prefix.As per coding guidelines, "Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/solve.cu` around lines 318 - 321, The code saves the original variable count in n_orig_before_sc but still builds callbacks and payloads using the reformulated/problem-after-SC dimension, causing GET_SOLUTION/SET_SOLUTION and run_mip callbacks to expose auxiliary semi‑continuous binaries; update the callback setup and all places that construct or consume solution vectors to map indices back to the original space after detail::reformulate_semi_continuous (using n_orig_before_sc and sc_used_fallback_big_m) so only the first n_orig_before_sc variables (and any mapped originals) are sent/received, trimming or translating SC auxiliary indices into the original variable indices in GET_SOLUTION/SET_SOLUTION handlers and in run_mip callback payload construction.cpp/src/mip_heuristics/presolve/semi_continuous.cu (1)
102-110:⚠️ Potential issue | 🟠 MajorHonor the caller’s time limit during SC bound strengthening.
bp_settings.time_limit = 5.0lets this phase run for up to five seconds even whensettings.time_limitis smaller, so SC reformulation can consume the solve budget before the main solve path starts. Please clamp this to the remaining user budget instead of a fixed cap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/presolve/semi_continuous.cu` around lines 102 - 110, The hardcoded bp_settings.time_limit = 5.0 lets SC bound strengthening ignore the caller's remaining time; change it to clamp against the user's remaining budget (e.g., compute a remaining_budget value from the solver settings or elapsed time and set bp_settings.time_limit = std::max(0.0, std::min(5.0, remaining_budget))). Update the code around bound_presolve_t<i_t, f_t> bps(...) before bps.solve(temp_pb) so the bp_settings.time_limit uses that clamped remaining budget (ensure you handle absence of remaining_budget by falling back to settings.time_limit and keep the value non-negative).
🧹 Nitpick comments (2)
cpp/CMakeLists.txt (1)
292-297: Remove duplicated OpenSSL discovery logic.This block is duplicated later at Lines 304-309 with the same conditions/body. Keeping both increases drift risk; keep one shared check before gRPC/Protobuf resolution.
♻️ Suggested cleanup
if(NOT TARGET OpenSSL::SSL) find_package(OpenSSL CONFIG QUIET) if(NOT OpenSSL_FOUND AND NOT OPENSSL_FOUND) find_package(OpenSSL REQUIRED) endif() endif() # gRPC is required for this branch - it provides remote execution features # gRPC can come from either: # - an installed CMake package (gRPCConfig.cmake), or # - an in-tree build (e.g. python/libcuopt uses FetchContent(grpc), which defines gRPC::grpc++). - -if(NOT TARGET OpenSSL::SSL) - find_package(OpenSSL CONFIG QUIET) - if(NOT OpenSSL_FOUND AND NOT OPENSSL_FOUND) - find_package(OpenSSL REQUIRED) - endif() -endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/CMakeLists.txt` around lines 292 - 297, Remove the duplicated OpenSSL discovery block: keep a single check that tests if(NOT TARGET OpenSSL::SSL) and then calls find_package(OpenSSL CONFIG QUIET) with the fallback find_package(OpenSSL REQUIRED) if not found; delete the later repeated block and ensure the remaining block appears once before gRPC/Protobuf resolution so OpenSSL::SSL is discovered centrally for subsequent targets.cpp/tests/mip/semi_continuous_test.cu (1)
65-118: Add a regression for the fallback-sc_big_mguard.These cases cover valid SC models and invalid bounds, but they never assert the new runtime path where an infinite-UB SC variable remains active at fallback
sc_big_m. A targeted case here would protectcpp/src/mip_heuristics/solve.cu:595-611from silently regressing.As per coding guidelines, "
**/*test*.{cpp,cu,py}: Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/tests/mip/semi_continuous_test.cu` around lines 65 - 118, Add a targeted unit test that exercises the fallback sc_big_m path for semi‑continuous variables: create a test (e.g., TEST(mip_solve, semi_continuous_fallback_sc_big_m)) that builds a problem with an infinite/unbounded upper bound for the SC variable using make_sc_problem (or directly construct the problem), call solve_mip(problem, settings), assert solver returns a valid termination status (Optimal or appropriate status) and validate numerical correctness of the solution/objective (not just no-error), and additionally assert that the fallback big‑M path was taken indirectly by checking the SC variable value is consistent with treating UB as very large (compare host_solution[0] against expected value within tolerance) so cpp/src/mip_heuristics/solve.cu's sc_big_m guard is exercised; reuse existing settings/time_limit and error-checking patterns from semi_continuous_regressions and semi_continuous_invalid_bounds_rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/mip_heuristics/presolve/semi_continuous.cu`:
- Around line 183-190: The code sets U = big_m when std::isfinite(U) is false
but does not validate big_m (settings.sc_big_m) against finiteness or L, which
can make the reformulation infeasible; update the logic around U (and the
fallback big_m) to first check that settings.sc_big_m is finite and greater than
or equal to L before assigning U = big_m, otherwise reject/clamp it (e.g., leave
U as +inf or set U = max(L, min(big_m, some_safe_max))) and avoid writing -U
into the linking row if invalid; also set/clear used_fallback_big_m[idx] only
when a valid fallback was applied so callers know whether a safe value was used.
In `@cpp/src/mip_heuristics/solve.cu`:
- Around line 322-327: The code currently clears settings.initial_solutions
after check_initial_solution_representation() runs, which allows invalid SC warm
starts to be validated and potentially cause failures; before calling
check_initial_solution_representation() ensure semi-continuous (had_sc) models
do not run validation by either clearing settings.initial_solutions when had_sc
is true (as done in the CUOPT_LOG_WARN branch) prior to any validation, or
adding a guard inside check_initial_solution_representation() to skip validation
when had_sc is true; update the logic around had_sc, settings.initial_solutions,
and the check_initial_solution_representation() call so SC warm starts are
dropped before validation and keep the CUOPT_LOG_WARN message referencing
settings.initial_solutions.size().
---
Duplicate comments:
In `@cpp/src/mip_heuristics/presolve/semi_continuous.cu`:
- Around line 102-110: The hardcoded bp_settings.time_limit = 5.0 lets SC bound
strengthening ignore the caller's remaining time; change it to clamp against the
user's remaining budget (e.g., compute a remaining_budget value from the solver
settings or elapsed time and set bp_settings.time_limit = std::max(0.0,
std::min(5.0, remaining_budget))). Update the code around bound_presolve_t<i_t,
f_t> bps(...) before bps.solve(temp_pb) so the bp_settings.time_limit uses that
clamped remaining budget (ensure you handle absence of remaining_budget by
falling back to settings.time_limit and keep the value non-negative).
In `@cpp/src/mip_heuristics/solve.cu`:
- Around line 318-321: The code saves the original variable count in
n_orig_before_sc but still builds callbacks and payloads using the
reformulated/problem-after-SC dimension, causing GET_SOLUTION/SET_SOLUTION and
run_mip callbacks to expose auxiliary semi‑continuous binaries; update the
callback setup and all places that construct or consume solution vectors to map
indices back to the original space after detail::reformulate_semi_continuous
(using n_orig_before_sc and sc_used_fallback_big_m) so only the first
n_orig_before_sc variables (and any mapped originals) are sent/received,
trimming or translating SC auxiliary indices into the original variable indices
in GET_SOLUTION/SET_SOLUTION handlers and in run_mip callback payload
construction.
---
Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Around line 292-297: Remove the duplicated OpenSSL discovery block: keep a
single check that tests if(NOT TARGET OpenSSL::SSL) and then calls
find_package(OpenSSL CONFIG QUIET) with the fallback find_package(OpenSSL
REQUIRED) if not found; delete the later repeated block and ensure the remaining
block appears once before gRPC/Protobuf resolution so OpenSSL::SSL is discovered
centrally for subsequent targets.
In `@cpp/tests/mip/semi_continuous_test.cu`:
- Around line 65-118: Add a targeted unit test that exercises the fallback
sc_big_m path for semi‑continuous variables: create a test (e.g.,
TEST(mip_solve, semi_continuous_fallback_sc_big_m)) that builds a problem with
an infinite/unbounded upper bound for the SC variable using make_sc_problem (or
directly construct the problem), call solve_mip(problem, settings), assert
solver returns a valid termination status (Optimal or appropriate status) and
validate numerical correctness of the solution/objective (not just no-error),
and additionally assert that the fallback big‑M path was taken indirectly by
checking the SC variable value is consistent with treating UB as very large
(compare host_solution[0] against expected value within tolerance) so
cpp/src/mip_heuristics/solve.cu's sc_big_m guard is exercised; reuse existing
settings/time_limit and error-checking patterns from semi_continuous_regressions
and semi_continuous_invalid_bounds_rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 91215217-c7a2-4ff5-bed8-e8c9965e190d
📒 Files selected for processing (8)
cpp/CMakeLists.txtcpp/src/mip_heuristics/CMakeLists.txtcpp/src/mip_heuristics/presolve/semi_continuous.cucpp/src/mip_heuristics/presolve/semi_continuous.cuhcpp/src/mip_heuristics/solve.cucpp/tests/mip/CMakeLists.txtcpp/tests/mip/semi_continuous_test.cudatasets/mip/sc_inferred_ub.mps
✅ Files skipped from review due to trivial changes (1)
- datasets/mip/sc_inferred_ub.mps
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/mip_heuristics/CMakeLists.txt
Comment on lines
+183
to
+190
| if (!std::isfinite(U)) { | ||
| U = big_m; | ||
| CUOPT_LOG_DEBUG( | ||
| "Semi-continuous var %d has no finite upper bound after bounds " | ||
| "strengthening; using fallback sc_big_m %.6g", | ||
| idx, | ||
| static_cast<double>(big_m)); | ||
| if (used_fallback_big_m != nullptr) { (*used_fallback_big_m)[idx] = uint8_t{1}; } |
There was a problem hiding this comment.
Validate sc_big_m before turning it into a hard upper bound.
If settings.sc_big_m is non-finite or smaller than L, the reformulation changes the feasible set: x >= L*b and x <= U*b make the b = 1 branch impossible even when the original model had feasible positive values. Reject or clamp invalid fallback values before writing -U into the linking row.
As per coding guidelines, "Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/presolve/semi_continuous.cu` around lines 183 - 190,
The code sets U = big_m when std::isfinite(U) is false but does not validate
big_m (settings.sc_big_m) against finiteness or L, which can make the
reformulation infeasible; update the logic around U (and the fallback big_m) to
first check that settings.sc_big_m is finite and greater than or equal to L
before assigning U = big_m, otherwise reject/clamp it (e.g., leave U as +inf or
set U = max(L, min(big_m, some_safe_max))) and avoid writing -U into the linking
row if invalid; also set/clear used_fallback_big_m[idx] only when a valid
fallback was applied so callers know whether a safe value was used.
Comment on lines
+322
to
+327
| if (had_sc && !settings.initial_solutions.empty()) { | ||
| CUOPT_LOG_WARN( | ||
| "Ignoring %zu user initial solution(s): semi-continuous warm starts are not supported yet", | ||
| settings.initial_solutions.size()); | ||
| settings.initial_solutions.clear(); | ||
| } |
There was a problem hiding this comment.
Drop unsupported SC warm starts before validating them.
These initial solutions are only cleared here, after check_initial_solution_representation() has already run on Line 312. An invalid warm start can still fail the solve even though this branch later ignores it. Please skip validation for SC models or clear the warm starts before the checker runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/solve.cu` around lines 322 - 327, The code currently
clears settings.initial_solutions after check_initial_solution_representation()
runs, which allows invalid SC warm starts to be validated and potentially cause
failures; before calling check_initial_solution_representation() ensure
semi-continuous (had_sc) models do not run validation by either clearing
settings.initial_solutions when had_sc is true (as done in the CUOPT_LOG_WARN
branch) prior to any validation, or adding a guard inside
check_initial_solution_representation() to skip validation when had_sc is true;
update the logic around had_sc, settings.initial_solutions, and the
check_initial_solution_representation() call so SC warm starts are dropped
before validation and keep the CUOPT_LOG_WARN message referencing
settings.initial_solutions.size().
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
1 participant
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.
Closes: #1059