Conversation
![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
♻️ Duplicate comments (2)
cpp/src/mip_heuristics/diversity/diversity_manager.cu (1)
104-107:⚠️ Potential issue | 🟡 MinorValidate the lower bound of
CUOPT_CONFIG_IDtoo.At Line 104, only
env_config_id >= max_configis checked; negative IDs still pass despite the stated valid range[0, max_config).Suggested patch
- if (max_config > 0 && env_config_id >= max_config) { + if (max_config > 0 && (env_config_id < 0 || env_config_id >= max_config)) { CUOPT_LOG_WARN( "CUOPT_CONFIG_ID=%d is outside [0, %d). Ignoring cut override.", env_config_id, max_config); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/diversity/diversity_manager.cu` around lines 104 - 107, The current guard only checks env_config_id >= max_config and misses negative values; update the validation around max_config and env_config_id so it rejects any env_config_id outside [0, max_config) by checking env_config_id < 0 || env_config_id >= max_config, and log the same CUOPT_LOG_WARN message before returning; locate the check using the symbols max_config, env_config_id, and CUOPT_LOG_WARN in diversity_manager.cu to implement the fix.cpp/src/cuts/cuts.cpp (1)
99-105:⚠️ Potential issue | 🟡 MinorRemove unreachable code after early returns in contradictory-literal handling.
After the
return clique_cut_build_status_t::NO_CUT;on Line 101 and Line 114, the debug log andINFEASIBLEreturns are dead code and never execute.Suggested cleanup
if (complement) { if (seen_original.count(var_idx) > 0) { // FIXME: this is temporary, fix all the vars of all other vars in the clique return clique_cut_build_status_t::NO_CUT; - CLIQUE_CUTS_DEBUG("build_clique_cut infeasible var=%lld appears as variable and complement", - static_cast<long long>(var_idx)); - return clique_cut_build_status_t::INFEASIBLE; } @@ } else { if (seen_complement.count(var_idx) > 0) { // FIXME: this is temporary, fix all the vars of all other vars in the clique return clique_cut_build_status_t::NO_CUT; - CLIQUE_CUTS_DEBUG("build_clique_cut infeasible var=%lld appears as variable and complement", - static_cast<long long>(var_idx)); - return clique_cut_build_status_t::INFEASIBLE; }Also applies to: 112-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 99 - 105, In build_clique_cut, there are unreachable statements after early returns when detecting a contradictory literal (the seen_original.count(var_idx) check): remove the dead CLIQUE_CUTS_DEBUG(...) call and the subsequent return clique_cut_build_status_t::INFEASIBLE that come after return clique_cut_build_status_t::NO_CUT; and do the same cleanup for the analogous block around the other occurrence (the lines corresponding to the second contradictory-literal handling); keep the single return (NO_CUT) and ensure no code remains after it so seen_original, var_idx and clique_cut_build_status_t logic is not followed by unreachable logging/returns.
🧹 Nitpick comments (5)
cpp/src/cuts/cuts.hpp (1)
164-171: Move the test-only clique helper to a test-facing boundary.Declaring
find_maximal_cliques_for_test(...)in the production header increases surface area and coupling. Prefer a test utility header (or guarded exposure) to keep production interfaces tighter.Based on learnings: "Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.hpp` around lines 164 - 171, The test-only declaration find_maximal_cliques_for_test(...) is exposed in the production header (cuts.hpp), increasing coupling; move this declaration into a dedicated test utility header (e.g., cuts_test_utils.hpp) or wrap it with a test-only guard so production code no longer includes it. Update build/test targets to include the new header where tests need it and remove the declaration from cuts.hpp; keep the original signature and any required includes so tests link against the same implementation without widening the public API.cpp/src/branch_and_bound/branch_and_bound.hpp (1)
75-79: Consider decouplingbranch_and_bound_tfrom cut-specific detail types.
branch_and_bound_tnow directly depends ondetail::clique_table_t. A narrower interface/opaque provider would keep B&B easier to reuse and test independently from cut internals.Based on learnings: "Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms".
Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.hpp` around lines 75 - 79, The constructor branch_and_bound_t(...) currently takes a concrete detail::clique_table_t, creating tight coupling to cut internals; change the API to accept an abstract/opaque provider instead (e.g., a std::shared_ptr to an interface like iclique_table_t or a std::function/visitor that exposes only the minimal operations B&B needs), update the constructor signature and all uses of detail::clique_table_t in branch_and_bound_t (and the other occurrence referencing clique_table_t) to the new interface type, and adapt construction sites to wrap or convert the existing detail::clique_table_t into that interface so the branch_and_bound_t implementation depends only on the narrow provider contract.cpp/src/branch_and_bound/branch_and_bound.cpp (2)
2108-2123: Use debug-level logging for metric skip paths.The “cut gap closure skipped …” messages are emitted on normal non-benchmark runs and can add noise. Prefer debug-level logging for these branches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2108 - 2123, The two informational skip messages emitted via settings_.log.printf (the "Cut gap closure skipped: max_cut_passes=..." and the "Cut gap closure skipped: no objective reference..." calls in branch_and_bound.cpp) should be lowered to debug level to avoid noise; replace those settings_.log.printf calls with the logger's debug-level API (e.g., settings_.log.debug(...) or settings_.log.printf with a DEBUG level) while keeping the same formatted text and arguments, so the messages are only shown at debug verbosity.
24-24: Extract cut-gap-closure reporting fromsolve()and isolate lookup dependency.The inline metric lambda and direct include of known-objective lookup logic make
solve()heavier and couple B&B to diversity utilities. Moving this into a small helper/reporting unit would improve cohesion.Based on learnings: "Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms".
Also applies to: 2102-2144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` at line 24, The solve() function currently inlines a metric lambda and directly depends on known_miplib_objectives.hpp; extract that inline metric and the lookup logic into a new small helper/reporting unit (e.g., create a function like generateCutGapClosureReport or reportCutGapClosure) and replace the lambda/include in solve() with a call to that helper; move the `#include` <mip_heuristics/diversity/known_miplib_objectives.hpp> out of branch_and_bound.cpp into the new helper's implementation file so branch-and-bound no longer directly depends on the diversity utility, and update references in solve() (and the code block around the 2102-2144 region) to use the new helper function.cpp/src/cuts/cuts.cpp (1)
1119-1139: Decouple clique-table construction from per-pass cut separation.
generate_clique_cuts()currently owns lazy clique-table building and tolerance translation, which tightly couples preprocessing and cut separation. Consider injecting a prebuilt/owned clique-table context into this stage and rebuilding only on problem changes.Based on learnings "Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 1119 - 1139, generate_clique_cuts currently builds and configures the clique table (clique_table_, detail::build_clique_table) and translates solver tolerances inline, coupling preprocessing with per-pass cut separation; refactor by extracting clique-table construction and tolerances mapping into an injected/owned context that generate_clique_cuts consumes: create or accept a prebuilt clique table object (or a lightweight clique_table_provider) that is updated only when the problem changes, move the tolerances population logic (the tolerances variable mapping from settings.primal_tol, settings.zero_tol, settings.integer_tol, absolute_mip_gap_tol, relative_mip_gap_tol) into that provider or a helper (e.g., clique_table_builder or clique_context) and call detail::build_clique_table only from that component; update generate_clique_cuts to check/use clique_table_ without rebuilding and to rely on an external timer/timeout decision so per-pass separation no longer triggers reconstruction.
🤖 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/cuts/cuts.cpp`:
- Around line 72-76: The build_clique_cut function validates lower_bounds and
xstar sizes but omits checks for vectors accessed by var_idx; add size
assertions to ensure static_cast<size_t>(num_vars) <= upper_bounds.size() and
static_cast<size_t>(num_vars) <= var_types.size() (and any other per-variable
vectors used in the function) before the loop that uses var_idx, mirroring the
existing cuopt_assert pattern, and replicate the same checks around the other
block referenced (lines 88-92) so all indexed accesses are guarded.
- Around line 1154-1166: generate_clique_cuts() assumes reduced_costs has at
least num_vars elements but never validates it, causing extend_clique_vertices()
to potentially read out-of-bounds via reduced_costs[var_idx % num_vars]; add a
guard in generate_clique_cuts() that checks reduced_costs.size() >=
static_cast<size_t>(num_vars) (or handle smaller sizes gracefully) and either
assert/log+return/throw an error before calling extend_clique_vertices(), and
apply the same check at the other mentioned location (around lines where
generate_clique_cuts() is called, e.g. the second site noted) to ensure safe
indexing.
---
Duplicate comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 99-105: In build_clique_cut, there are unreachable statements
after early returns when detecting a contradictory literal (the
seen_original.count(var_idx) check): remove the dead CLIQUE_CUTS_DEBUG(...) call
and the subsequent return clique_cut_build_status_t::INFEASIBLE that come after
return clique_cut_build_status_t::NO_CUT; and do the same cleanup for the
analogous block around the other occurrence (the lines corresponding to the
second contradictory-literal handling); keep the single return (NO_CUT) and
ensure no code remains after it so seen_original, var_idx and
clique_cut_build_status_t logic is not followed by unreachable logging/returns.
In `@cpp/src/mip_heuristics/diversity/diversity_manager.cu`:
- Around line 104-107: The current guard only checks env_config_id >= max_config
and misses negative values; update the validation around max_config and
env_config_id so it rejects any env_config_id outside [0, max_config) by
checking env_config_id < 0 || env_config_id >= max_config, and log the same
CUOPT_LOG_WARN message before returning; locate the check using the symbols
max_config, env_config_id, and CUOPT_LOG_WARN in diversity_manager.cu to
implement the fix.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2108-2123: The two informational skip messages emitted via
settings_.log.printf (the "Cut gap closure skipped: max_cut_passes=..." and the
"Cut gap closure skipped: no objective reference..." calls in
branch_and_bound.cpp) should be lowered to debug level to avoid noise; replace
those settings_.log.printf calls with the logger's debug-level API (e.g.,
settings_.log.debug(...) or settings_.log.printf with a DEBUG level) while
keeping the same formatted text and arguments, so the messages are only shown at
debug verbosity.
- Line 24: The solve() function currently inlines a metric lambda and directly
depends on known_miplib_objectives.hpp; extract that inline metric and the
lookup logic into a new small helper/reporting unit (e.g., create a function
like generateCutGapClosureReport or reportCutGapClosure) and replace the
lambda/include in solve() with a call to that helper; move the `#include`
<mip_heuristics/diversity/known_miplib_objectives.hpp> out of
branch_and_bound.cpp into the new helper's implementation file so
branch-and-bound no longer directly depends on the diversity utility, and update
references in solve() (and the code block around the 2102-2144 region) to use
the new helper function.
In `@cpp/src/branch_and_bound/branch_and_bound.hpp`:
- Around line 75-79: The constructor branch_and_bound_t(...) currently takes a
concrete detail::clique_table_t, creating tight coupling to cut internals;
change the API to accept an abstract/opaque provider instead (e.g., a
std::shared_ptr to an interface like iclique_table_t or a std::function/visitor
that exposes only the minimal operations B&B needs), update the constructor
signature and all uses of detail::clique_table_t in branch_and_bound_t (and the
other occurrence referencing clique_table_t) to the new interface type, and
adapt construction sites to wrap or convert the existing detail::clique_table_t
into that interface so the branch_and_bound_t implementation depends only on the
narrow provider contract.
In `@cpp/src/cuts/cuts.cpp`:
- Around line 1119-1139: generate_clique_cuts currently builds and configures
the clique table (clique_table_, detail::build_clique_table) and translates
solver tolerances inline, coupling preprocessing with per-pass cut separation;
refactor by extracting clique-table construction and tolerances mapping into an
injected/owned context that generate_clique_cuts consumes: create or accept a
prebuilt clique table object (or a lightweight clique_table_provider) that is
updated only when the problem changes, move the tolerances population logic (the
tolerances variable mapping from settings.primal_tol, settings.zero_tol,
settings.integer_tol, absolute_mip_gap_tol, relative_mip_gap_tol) into that
provider or a helper (e.g., clique_table_builder or clique_context) and call
detail::build_clique_table only from that component; update generate_clique_cuts
to check/use clique_table_ without rebuilding and to rely on an external
timer/timeout decision so per-pass separation no longer triggers reconstruction.
In `@cpp/src/cuts/cuts.hpp`:
- Around line 164-171: The test-only declaration
find_maximal_cliques_for_test(...) is exposed in the production header
(cuts.hpp), increasing coupling; move this declaration into a dedicated test
utility header (e.g., cuts_test_utils.hpp) or wrap it with a test-only guard so
production code no longer includes it. Update build/test targets to include the
new header where tests need it and remove the declaration from cuts.hpp; keep
the original signature and any required includes so tests link against the same
implementation without widening the public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47d8a614-f387-494a-94bd-7b2b69b0111c
📒 Files selected for processing (5)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hppcpp/src/mip_heuristics/diversity/diversity_manager.cu
| cuopt_assert(num_vars > 0, "Clique cut num_vars must be positive"); | ||
| cuopt_assert(static_cast<size_t>(num_vars) <= lower_bounds.size(), | ||
| "Clique cut lower bounds size mismatch"); | ||
| cuopt_assert(static_cast<size_t>(num_vars) <= xstar.size(), "Clique cut xstar size mismatch"); | ||
|
|
There was a problem hiding this comment.
Add missing size checks for vectors indexed by var_idx in build_clique_cut.
The function checks lower_bounds and xstar, but also indexes upper_bounds[var_idx] and var_types[var_idx] without corresponding size validation.
Suggested hardening
cuopt_assert(static_cast<size_t>(num_vars) <= lower_bounds.size(),
"Clique cut lower bounds size mismatch");
+ cuopt_assert(static_cast<size_t>(num_vars) <= upper_bounds.size(),
+ "Clique cut upper bounds size mismatch");
+ cuopt_assert(static_cast<size_t>(num_vars) <= var_types.size(),
+ "Clique cut var_types size mismatch");
cuopt_assert(static_cast<size_t>(num_vars) <= xstar.size(), "Clique cut xstar size mismatch");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)".
Also applies to: 88-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/cuts/cuts.cpp` around lines 72 - 76, The build_clique_cut function
validates lower_bounds and xstar sizes but omits checks for vectors accessed by
var_idx; add size assertions to ensure static_cast<size_t>(num_vars) <=
upper_bounds.size() and static_cast<size_t>(num_vars) <= var_types.size() (and
any other per-variable vectors used in the function) before the loop that uses
var_idx, mirroring the existing cuopt_assert pattern, and replicate the same
checks around the other block referenced (lines 88-92) so all indexed accesses
are guarded.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cu (1)
1052-1063: Consider simplifying dual-storage pattern.The current approach creates either a local
clique_table_localor ashared_ptrbased on whether output is requested. While correct, this could be simplified by always usingshared_ptrinternally and optionally exposing it:auto clique_table = std::make_shared<clique_table_t<i_t, f_t>>(...); // use clique_table.get() throughout if (clique_table_out != nullptr) { *clique_table_out = clique_table; }However, the current implementation works correctly and the local optimization avoids heap allocation when output isn't needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cu` around lines 1052 - 1063, The code uses a dual-storage pattern with clique_table_local, clique_table_shared and clique_table_ptr; simplify by always creating a std::shared_ptr<clique_table_t<i_t,f_t>> (e.g. auto clique_table) for internal use, replace uses of clique_table_ptr with clique_table.get(), and if clique_table_out != nullptr assign *clique_table_out = clique_table so callers still receive the shared_ptr; remove clique_table_local and clique_table_shared variables after this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cu`:
- Around line 1052-1063: The code uses a dual-storage pattern with
clique_table_local, clique_table_shared and clique_table_ptr; simplify by always
creating a std::shared_ptr<clique_table_t<i_t,f_t>> (e.g. auto clique_table) for
internal use, replace uses of clique_table_ptr with clique_table.get(), and if
clique_table_out != nullptr assign *clique_table_out = clique_table so callers
still receive the shared_ptr; remove clique_table_local and clique_table_shared
variables after this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41442d3c-ce71-41ab-8217-aa9753009f7a
📒 Files selected for processing (3)
cpp/src/cuts/cuts.cppcpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cucpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cuh
There was a problem hiding this comment.
The only concern is the increase in runtime. Ideally, we would see that this is performance neutral or speeds things up. If it is a slow down, I think we should merge, but keep it disabled.
Approving changes to branch_and_bound.{cpp,hpp} and cuts.{cpp, hpp} and simplex_solver_settings.hpp. Note I did not review the clique cut specific code, only how it interfaced with the existing code.
One thought: cuts.cpp is getting pretty big. It might be good to put the clique cut specific functions in the files clique_cuts.{cpp, hpp} and I can separate out cuts into the additional files knapsack_cuts.{cpp, hpp}, mir_cuts.{cpp, hpp}, gomory_cuts.{cpp, hpp}, and chvatal_gomory_cuts.{cpp,hpp} in a follow up PR. But fine to leave as is and I can do the separation later.
| if (num_cpu_threads < 0) { num_cpu_threads = omp_get_max_threads() / n_gpus; } | ||
| if (num_cpu_threads < 0) { | ||
| num_cpu_threads = omp_get_max_threads() / n_gpus; | ||
| // std::ifstream smt_file("/sys/devices/system/cpu/smt/active"); |
There was a problem hiding this comment.
This is non-production code, I would rather merge it now as the time is a bit tight for this PR.
|
/merge |
3 participants
This PR adds clique cuts to the set of cuts we have. It uses the ideas from Preprocessing and Cutting Planes with Conflict Graphs, Brito et.al.
In a previous PR, I have added the clique table generation as a preprocessing step (currently doesn't change the problem). This PR builds on that by using the extended cliques during cut passes. At each cut pass, a subgraph induced by the fractinoal variables on the current relaxation solution is used to generate clique cuts. We want to generate the set of violated clique inequalities that have at least min violation (1e-6). The maximal cliques in the fractional subgraph is found by Bron Kerbosh dynamic programming algorithm. The found maximal cliques are extended with the suitable integer valued variables on the original conflict graph. The extension is guided by reduced cost of the variables at the current relaxation optimal. We try to extend the cliques with variables that have lower reduced cost. The reason is that it disturbs dual simplex less (causes fewer refactors and iterations for resolve). Also the variables with lower reduced costs are likely to be active in the next resolve. This also results in more dominant cliques preventing multiple smaller sized cliques in the next iterations.
Benchmark results:
MIP GAP wins/losses defined as at least 10% difference:
main wins (6):
Instance dir1 gap dir2 gap diff
app1-2 0.8077 1.1818 +0.3741
bab2 0.4148 0.6148 +0.2001
glass4 0.3333 0.4483 +0.1149
map16715-04 0.9269 1.4085 +0.4816
neos-4413714-turia 0.7027 0.8454 +0.1427
rail01 0.5476 0.7934 +0.2458
clique cuts wins (11):
Instance dir1 gap dir2 gap diff
30n20b8 0.7688 0.6667 +0.1021
bab6 0.9255 0.3611 +0.5643
buildingenergy 0.1706 0.0036 +0.1670
co-100 0.8410 0.7089 +0.1321
neos-3754480-nidda 20.3512 19.6495 +0.7017
neos-5188808-nattai 0.2032 0.0000 +0.2032
netdiversion 0.3710 0.1429 +0.2281
physiciansched3-3 0.2444 0.0269 +0.2175
rocII-5-11 1.0893 0.7762 +0.3131
satellites2-60-fs 13.5000 1.4167 +12.0833
sorrell3 9.5625 9.4375 +0.1250
Optimality wins/loses:
Wins
graphdraw-domain (443.68s)
neos-5188808-nattai (462.84s)
supportcase26 (537.66s)
Losses
dano3_3 (121.30s)
Time to optimality geomean (shifted geomean by 1.0):
on 50 common optimal instances
Common optimal instances: 50
Geometric mean of ratios (main/clique): 1.1925