Add flow cover cuts at root#1178
Open
hlinsen wants to merge 33 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds flow-cover cut support to the MIP solver (new cut type, settings, generation logic, integration into cut pipeline, and tests) and extends the LP benchmark script with recursive model discovery and a cut-mode flag to control cut-related CLI options. ChangesFlow Cover Cut Implementation & Integration
Benchmark CLI: recursive discovery & cut-mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/linear_programming/run_mps_files.sh`:
- Around line 359-364: The recursive file collection can produce duplicate
basenames that cause log files to be overwritten; update the code that
constructs per-model log filenames to include a path-unique component (e.g., use
the file's path relative to MPS_DIR with slashes replaced by safe separators or
use dirname+basename) so each entry in mps_files yields a unique log name when
RECURSIVE=true and --write-log-file is enabled; locate where run_mps_files.sh
iterates over the mps_files array and replace uses of basename (or "$(basename
"$file").log") with a deterministic path-derived filename (for example
"${file#$MPS_DIR/}" with non-alphanumerics normalized) to avoid clobbering logs.
In `@cpp/src/cuts/cuts.cpp`:
- Around line 951-953: The hardcoded thresholds tol and bound_tol in cuts.cpp
(const f_t tol = 1e-6; const f_t bound_tol = 1e-8;) should be replaced with the
solver's configured numerical tolerances; find where these are used (flow-cover
acceptance and downstream cut cutoff computations) and read the solver/LP
feasibility and bound tolerances instead of the literals (e.g., use the
simplex/LP object's feasibility epsilon and bound epsilon parameters). Update
all dependent comparisons that currently use tol or bound_tol so they derive
from those solver tolerances (and propagate scaling if needed), and add a short
comment noting these come from the solver's tolerance settings rather than being
hardcoded.
- Around line 3881-3888: The code assumes slack_map_[i] is valid and indexes
lp.lower/upper unconditionally; when slack_map_[i] == -1 (rows with
slack_count==0) this causes out-of-bounds access. Fix by guarding the lookup:
check if slack_map_[i] >= 0 before accessing lp.lower[slack_map_[i]] and
lp.upper[slack_map_[i]]; if negative, set slack_lower/slack_upper (and derived
sigma_slack_lower/sigma_slack_upper) to safe defaults (e.g., 0) or compute them
in a way consistent with slack_coeff_i==0 so the rest of the logic remains
correct. Apply the same guarded check and handling in the mirrored block around
lines 3981-3988 as well.
In `@cpp/tests/mip/cuts_test.cu`:
- Around line 1482-1498: The test utilities access fixed indices without
validating vector size; update single_node_flow_fractional_solution to check
num_cols >= 6 (or clamp/resize/throw) before writing xstar[0]..xstar[5], and
update single_node_flow_y_feasible to verify y.size() >= 3 before reading y[0],
y[1], y[2] (or return false/assert/throw with a clear message). Use the function
names single_node_flow_fractional_solution and single_node_flow_y_feasible and
include a clear failure mode (assert or exception) so an input-shape mismatch
yields a deterministic test failure rather than undefined behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21e0b82f-501c-4310-a5c3-f4986b9a676f
📒 Files selected for processing (9)
benchmarks/linear_programming/run_mps_files.shcpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/solver.cucpp/tests/mip/cuts_test.cu
Comment on lines
+359
to
+364
| if [ "$RECURSIVE" = true ]; then | ||
| mapfile -t mps_files < <(find "$MPS_DIR" -type f \( -name "*.mps" -o -name "*.MPS" -o -name "*.SIF" \) | sort) | ||
| else | ||
| # Gather .mps/.MPS and .SIF files in the directory | ||
| mapfile -t mps_files < <(ls "$MPS_DIR"/*.mps "$MPS_DIR"/*.MPS "$MPS_DIR"/*.SIF 2>/dev/null) | ||
| fi |
There was a problem hiding this comment.
Recursive mode can clobber per-model log files.
With --recursive, two models from different subdirectories can share the same basename, but the worker still writes logs to $(basename ...).log. The later run overwrites the earlier one, so benchmark results become incomplete whenever --write-log-file is enabled.
Suggested fix
- if [ "$WRITE_LOG_FILE" = true ]; then
- args="$args --log-file $OUTPUT_DIR/$(basename "${mps_file%.mps}").log"
- fi
+ if [ "$WRITE_LOG_FILE" = true ]; then
+ rel_model_path="${mps_file#$MPS_DIR/}"
+ safe_model_name="${rel_model_path//\//__}"
+ safe_model_name="${safe_model_name%.mps}"
+ safe_model_name="${safe_model_name%.MPS}"
+ safe_model_name="${safe_model_name%.SIF}"
+ args="$args --log-file $OUTPUT_DIR/${safe_model_name}.log"
+ fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/linear_programming/run_mps_files.sh` around lines 359 - 364, The
recursive file collection can produce duplicate basenames that cause log files
to be overwritten; update the code that constructs per-model log filenames to
include a path-unique component (e.g., use the file's path relative to MPS_DIR
with slashes replaced by safe separators or use dirname+basename) so each entry
in mps_files yields a unique log name when RECURSIVE=true and --write-log-file
is enabled; locate where run_mps_files.sh iterates over the mps_files array and
replace uses of basename (or "$(basename "$file").log") with a deterministic
path-derived filename (for example "${file#$MPS_DIR/}" with non-alphanumerics
normalized) to avoid clobbering logs.
Comment on lines
+1482
to
+1498
| std::vector<double> single_node_flow_fractional_solution(int num_cols) | ||
| { | ||
| std::vector<double> xstar(num_cols, 0.0); | ||
| xstar[0] = 1.0; | ||
| xstar[1] = 2.0 / 3.0; | ||
| xstar[2] = 1.0; | ||
| xstar[3] = 3.0; | ||
| xstar[4] = 4.0; | ||
| xstar[5] = 3.0; | ||
| return xstar; | ||
| } | ||
|
|
||
| bool single_node_flow_y_feasible(const std::vector<double>& y) | ||
| { | ||
| const double activity = y[0] + y[1] - y[2]; | ||
| return activity <= 4.0 + 1e-8; | ||
| } |
There was a problem hiding this comment.
Guard fixed-index writes/reads to avoid UB if LP column layout changes.
Line 1485–1490 and Line 1496 assume minimum vector sizes (6 and 3) without checks. If convert_user_problem output shape changes, this can become out-of-bounds and make the test crash/flap instead of failing with a clear assertion.
Suggested patch
std::vector<double> single_node_flow_fractional_solution(int num_cols)
{
+ cuopt_assert(num_cols >= 6, "Expected at least 6 columns for x0..x2,y0..y2");
std::vector<double> xstar(num_cols, 0.0);
xstar[0] = 1.0;
xstar[1] = 2.0 / 3.0;
xstar[2] = 1.0;
xstar[3] = 3.0;
xstar[4] = 4.0;
xstar[5] = 3.0;
return xstar;
}
bool single_node_flow_y_feasible(const std::vector<double>& y)
{
+ cuopt_assert(y.size() == 3, "Expected exactly 3 flow variables");
const double activity = y[0] + y[1] - y[2];
return activity <= 4.0 + 1e-8;
}As per coding guidelines, **/*.{cpp,cuh,cu,hpp,py}: Flag missing input validation at library and server boundaries.
Also applies to: 1579-1581
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/tests/mip/cuts_test.cu` around lines 1482 - 1498, The test utilities
access fixed indices without validating vector size; update
single_node_flow_fractional_solution to check num_cols >= 6 (or
clamp/resize/throw) before writing xstar[0]..xstar[5], and update
single_node_flow_y_feasible to verify y.size() >= 3 before reading y[0], y[1],
y[2] (or return false/assert/throw with a clear message). Use the function names
single_node_flow_fractional_solution and single_node_flow_y_feasible and include
a clear failure mode (assert or exception) so an input-shape mismatch yields a
deterministic test failure rather than undefined behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 1408-1410: Replace the two different hardcoded tolerances used
when checking SNF arc feasibility with a single configurable tolerance from
solver settings (e.g., use the solver's numeric tolerance parameter) and apply
it consistently to both the selection check (where best->arc.y_value and
best->arc.y_value - best->arc.u * best->arc.x_value are tested) and the
revalidation loop that currently uses a tighter literal; update the checks to
reference that single tolerance symbol and add a short inline
comment/documentation string noting the tolerance purpose so it isn't left as a
magic literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b8908d32-ea99-4ecb-a47d-5b5280153d6b
📒 Files selected for processing (2)
cpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/cuts/cuts.hpp
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
2 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.
Closes: #1065