Add flow cover cuts at root#1178
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
| 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.
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
| const f_t capacity = std::max<f_t>(0.0, spec.u); | ||
| if (capacity <= feasibility_tol) { return; } | ||
|
|
||
| snf_candidate_t<i_t, f_t> candidate; |
There was a problem hiding this comment.
Is snf single node flow? If so you might want to write out the acronyms to make it easier on the reader.
| i_t knapsack_row, | ||
| inequality_t<i_t, f_t>& cut); | ||
|
|
||
| i_t generate_flow_cover_cut(const lp_problem_t<i_t, f_t>& lp, |
There was a problem hiding this comment.
Would it make sense to collect all the flow cover code into a class flow_cover_generation_t?
| continue; | ||
| } | ||
|
|
||
| const f_t sigma_slack_lower = |
There was a problem hiding this comment.
Ugh. Sorry you have to deal with this as well. In 26.08 I want to fix it so that slack_coeff is always 1.
| }; | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| struct flow_cover_scratch_t { |
There was a problem hiding this comment.
If I understand correctly flow_cover_scratch_t is used to hold a bunch of intermediate variables that you need during the computation. Maybe you should just make flow_cover_scratch_t the flow_cover_generator_t and add methods to it?
| b = negate_row ? -row.rhs : row.rhs; | ||
| if (slack_count == 1) { | ||
| if (std::abs(slack_coeff) <= coefficient_tol) { return false; } | ||
| const f_t sigma_slack_lower = slack_coeff > 0.0 ? slack_coeff * context.lp.lower[slack_col] |
There was a problem hiding this comment.
Ugh. I'm so sorry about this. My poor design decision is leading to difficult code :(
| } | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| bool knapsack_generation_t<i_t, f_t>::select_flow_cover( |
There was a problem hiding this comment.
Is this where you are doing separation? If so you might want to call that out in the name.
| } | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| flow_cover_evaluation_t<f_t> knapsack_generation_t<i_t, f_t>::evaluate_sgfci( |
There was a problem hiding this comment.
What does sgfci stand for? Maybe you could write out the abbrevation?
|
|
||
| template <typename f_t> | ||
| struct flow_cover_selection_t { | ||
| f_t lambda; |
There was a problem hiding this comment.
It's a bit strange to have a struct with only a single variable? Can this be removed?
| }; | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| struct flow_cover_context_t { |
There was a problem hiding this comment.
Would it make sense to unify flow_cover_context_t and flow_cover_scratch_t?
| } | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| flow_cover_evaluation_t<f_t> knapsack_generation_t<i_t, f_t>::evaluate_cmirfci( |
There was a problem hiding this comment.
I know what cMIR stands for "complemented mixed integer rounding" what is fci? Flow cover inequality? You might want to write out at least some of this abbrevation to make it easier for the reader.
| std::sort(perm.begin(), perm.end(), [&](i_t i, i_t j) { return ratios[i] > ratios[j]; }); | ||
|
|
||
| if (mode == greedy_knapsack_mode_t::STRICT_RATIO_PREFIX) { | ||
| // Wolter Algorithm 5.1 for KPSNF^rat: take the ratio-sorted prefix while |
There was a problem hiding this comment.
Is the only difference between this code and the greedy selection below that we are stopping at the first item that does not fit?
If so, maybe you can just alter the code below as follows
for (i_t j : perm) {
if (weights[j] <= remaining) {
// ...
} else if (mode == greedy_knapsack_mode_t::STRICT_RATIO_PREFIX) {
break;
}
}
There was a problem hiding this comment.
Oh I see, there is also a strict comparison? So you have to be strictly under the limit.
It might still be possible to fold this into the code below.
If not, fine to keep it separate.
| // If num_neg_inf_[i] == 1 and var_types[s] != INTEGER, we can't derive a bound | ||
| // If num_neg_inf_[i] == 2 and var_types[s ^ j] != INTEGER, we can't derive a bound | ||
| // If num_neg_inf_[i] == 2 and var_types[s ^ j] == INTEGER, and lower_activity_j != -inf, | ||
| // we can't derive a bound |
There was a problem hiding this comment.
Can you restore the comment? If the comment is still valid?
Or if not replace it with a new one?
| } | ||
| f_t start_time = tic(); | ||
|
|
||
| struct variable_bound_edge_t { |
There was a problem hiding this comment.
Why do we need to change this for flow cover cuts?
| upper_variables.push_back(l); | ||
| upper_weights.push_back(-a_il / a_ij); | ||
| upper_biases.push_back(beta / a_ij - (1.0 / a_ij) * sum); | ||
| const variable_bound_edge_t edge{l, -a_il / a_ij, beta / a_ij - (1.0 / a_ij) * sum}; |
There was a problem hiding this comment.
Are these edges being used anywhere? If not, maybe we could revert back to the previous code?
| } | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| variable_bounds_t<i_t, f_t>::variable_bounds_t(const lp_problem_t<i_t, f_t>& lp, |
There was a problem hiding this comment.
I'm not sure I understand the changes being made here. Can you explain the goal of these changes?
| }; | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| struct snf_candidate_t { |
There was a problem hiding this comment.
snf -> single_node_flow?
| }; | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| struct snf_arc_t { |
There was a problem hiding this comment.
snf -> single_node_flow ?
3 participants
Closes: #1065