Zero Half(Odd-Cycle) cuts#1428
Open
akifcorduk wants to merge 58 commits into
Open
Conversation
Brings in upstream's unified OpenMP threading model (PR NVIDIA#1099) and other fixes (NVIDIA#1206 concurrent LP exception cleanup, NVIDIA#1214/NVIDIA#1216 destruction/ capture fixes) while preserving local work on the cut and clique stack. Conflict resolution highlights: - Drop std::future/std::async clique flow; adopt upstream's omp task + omp_atomic_t<bool> signal_extend pattern. - Drop modify_problem parameter from find_initial_cliques (we already removed the code that consumed it); adapt the omp-task call site in branch_and_bound::solve accordingly. - Take upstream's [this, &population] capture for the root-LP CPUFJ improvement callback; the new omp taskwait-before-destruction guarantee makes the prior context-lifetime fix unnecessary. - Take upstream's do_cut_pass refactor of the per-pass LP resolve loop; move our per-pass root_lp_with_cuts publish into do_cut_pass so the benchmark metric is still updated on early exits. - Keep our out-of-line omp_mutex_t definitions in omp_helpers.cpp; the enhanced omp_atomic_t with std::memory_order is taken from upstream.
This reverts commit 09bdecd.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a configurable zero-half cut family: new solver settings, cut-type enum/name, fractional conflict-subgraph caching, Dijkstra-based odd-cycle extraction and odd-wheel lifting, integration into the cut pass and B&B wiring, and unit + end-to-end tests. ChangesZero-half cut generation and integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/src/mip_heuristics/diversity/diversity_manager.cu (1)
523-603:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten the root-relaxation handoff contract.
root_lp_usableis only a buffer-shape check, but it is also being used as the gate forset_root_relaxation_solution_callback(...). That means B&B can still receive payloads this function does not actually treat as a valid root relaxation:PrimalInfeasible/DualInfeasiblestill satisfyroot_lp_usable, and the callback is populated fromlp_result.get_primal_solution()before Line 600 clamps PDLP bound violations. The downstreamsolve_root_relaxation()path consumes that callback as a crossover candidate, so this can hand crossover an infeasibility certificate or an out-of-bounds primal vector instead of a sanitized relaxation iterate.Use a separate
root_lp_handoffablegate for the callback, clamp/sanitize before marshalinghost_primal, and only hand off statuses that the crossover path is documented to accept. citeturn0search0🤖 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/src/mip_heuristics/diversity/diversity_manager.cu` around lines 523 - 603, The root-relaxation handoff must be gated by a new root_lp_handoffable flag and the primal must be sanitized before marshaling: compute root_lp_handoffable (start from root_lp_usable) and only set it true for termination statuses the crossover expects (e.g., pdlp_termination_status_t::Optimal, and optionally pdlp_termination_status_t::TimeLimit only if an LP feasibility check passes); run clamp_within_var_bounds(lp_optimal_solution, ...) and any lp_sol.compute_feasibility()/lp_sol.get_feasible() checks before copying/prior to calling set_root_relaxation_solution_callback; use lp_result.get_termination_status(), lp_result.get_primal_solution()/get_dual_solution()/get_reduced_cost(), clamp_within_var_bounds, and set_root_relaxation_solution_callback to locate where to implement the flag, the sanitize step, and the tightened conditional.cpp/src/mip_heuristics/solver.cu (1)
430-455:⚠️ Potential issue | 🟡 MinorUpdate clique-table lifecycle comment (dead publish callback)
In
cpp/src/mip_heuristics/solver.cuthe lifecycle text claims B&B async-builds aclique_tableand publishes it intocontext.problem_ptr->clique_tableviaset_clique_publish_callback, but the entire callback block is commented out and there are no real (non-comment)publish_clique_table/ensure_clique_datareaders usingproblem_ptr->clique_table. B&B instead owns/buildsclique_table_internally (find_initial_cliques(..., &clique_table_, ...)) and passes it tocut_generation, so the “publish-to-heuristics” description should be removed or rewritten to match the actual ownership/consumption path.🤖 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/src/mip_heuristics/solver.cu` around lines 430 - 455, The comment about an async B&B publish of a clique_table is outdated: remove or rewrite the paragraph describing set_clique_publish_callback/publish_clique_table/ensure_clique_data since the callback is commented out and B&B actually owns/builds clique_table_ (via find_initial_cliques(..., &clique_table_, ...)) and passes it directly into cut_generation; also delete the commented-out callback block referencing set_clique_publish_callback to avoid misleading future readers and update the text to state that branch_and_bound_t maintains clique_table_ internally and supplies it to cut_generation rather than publishing it to context.problem_ptr->clique_table.
🧹 Nitpick comments (2)
benchmarks/linear_programming/cuopt/miplib2017_optima.hpp (1)
73-73: ⚡ Quick winRename getter functions to follow snake_case convention.
kBenchmarkOptima()andkBenchmarkInfeasible()usekCamelCasenaming, but the coding guidelines requiresnake_casefor all functions and methods. Consider renaming tobenchmark_optima()andbenchmark_infeasible()(or similar) to match project conventions.♻️ Proposed naming fix
-inline const std::unordered_map<std::string, double>& kBenchmarkOptima() +inline const std::unordered_map<std::string, double>& benchmark_optima() { static const std::unordered_map<std::string, double> kOptima = {-inline const std::unordered_set<std::string>& kBenchmarkInfeasible() +inline const std::unordered_set<std::string>& benchmark_infeasible() { static const std::unordered_set<std::string> kInfeas = {Then update the call sites on lines 333 and 341:
- const auto& m = kBenchmarkOptima(); + const auto& m = benchmark_optima();- return kBenchmarkInfeasible().count(normalize_instance_name(filename)) != 0; + return benchmark_infeasible().count(normalize_instance_name(filename)) != 0;Also applies to: 317-317
🤖 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/cuopt/miplib2017_optima.hpp` at line 73, The function names kBenchmarkOptima() and kBenchmarkInfeasible() violate the project's snake_case convention; rename these functions to benchmark_optima() and benchmark_infeasible() (and update their declarations and definitions accordingly) and update all call sites that reference kBenchmarkOptima and kBenchmarkInfeasible to the new names; ensure the return types and const/ref qualifiers remain unchanged and run a build to catch any remaining references.Source: Coding guidelines
cpp/include/cuopt/linear_programming/mip/solver_settings.hpp (1)
132-132: ⚡ Quick winDocument the new
zero_half_cutsparameter.The new parameter lacks a comment explaining its meaning, valid values, and behavioral implications. While this is consistent with other cut-setting members (lines 127–131, 133–134), documenting it would improve usability. From the internal contract (cpp/src/dual_simplex/simplex_solver_settings.hpp:83–184), the semantics are:
-1= automatic,0= disabled,>0= enabled. Additionally, enabling zero-half cuts (!= 0) triggers async clique-table preparation when thread count is sufficient (cpp/src/branch_and_bound/branch_and_bound.cpp:2451–2472), similar toclique_cuts.Consider adding a brief inline comment or a Doxygen block above the cut-settings group documenting the shared
-1/0/>0convention and any async-preparation behavior users should be aware of.🤖 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/include/cuopt/linear_programming/mip/solver_settings.hpp` at line 132, Add a Doxygen-style comment for the cut-settings group (or immediately above the member) explaining the semantics of zero_half_cuts: zero_half_cuts uses the same convention as other cut settings where -1 = automatic, 0 = disabled, >0 = enabled (positive values control aggressiveness/limit), and note that enabling zero-half cuts (value != 0) triggers asynchronous clique-table preparation when thread counts are sufficient (same behavior as clique_cuts); reference the member name zero_half_cuts and the related clique_cuts setting in the comment so maintainers can locate the linked behavior.
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Around line 2654-2663: The lambda publish_cut_generation_time is defined but
never invoked, so clamping and the force-time-limit logic are skipped; replace
the direct assignments that set
settings_.benchmark_info_ptr->cut_generation_time_sec using raw
toc(cut_generation_start_time) (including occurrences around the nearby blocks
you noted) with calls to publish_cut_generation_time(), and where the logic
should honor the time_limit use publish_cut_generation_time(true). Ensure calls
occur after computing cut_generation_start_time and before any early returns so
the lambda (which uses toc, settings_.benchmark_info_ptr, and
settings_.time_limit) performs the clamping and publishes the final double
value.
In `@cpp/src/cuts/cuts.cpp`:
- Around line 1077-1119: The code currently uses the already_used vector in the
main source loop (for (int s = 0; s < num_local; ++s)) and marks vertices as
used after pushing a cycle (setting already_used[v] = 1), which causes later
sources to be skipped if they share vertices with an earlier found cycle; to fix
this, remove the already_used gating (the if (already_used[s]) continue;) and
the marking loop that sets already_used[v] = 1 so that dijkstra_odd_cycle and
the helper will return all violated odd cycles including overlapping ones; if
the disjoint-only behavior is intended instead, rename already_used to something
explicit and add documentation/comments around dijkstra_odd_cycle and the result
collection to make that contract clear.
- Around line 3222-3233: The loop is pushing raw xstar[j] and 1-xstar[j] into
sub_cg_ even when xstar is slightly outside [0,1]; clamp these cached literal
weights to [0,1] (or skip/report if they exceed primal_tol) before pushing into
sub_cg_. Concretely, in the block that computes const f_t xj = xstar[j] inside
the for-loop over j (and after the integer_tol check), compute a clamped value
(e.g., xj_clamped = clamp(xj, 0.0, 1.0)) and use that for
sub_cg_.weights.push_back(...) and for 1 - xj_clamped; if xj is outside [0 -
primal_tol, 1 + primal_tol], treat it as an error/skip per project policy
instead of caching the raw value.
In `@cpp/src/cuts/cuts.hpp`:
- Around line 360-363: Update the setter set_clique_cousin_minhash_k(i_t v) to
validate the input and reject non-positive values: if v <= 0, throw a
descriptive std::invalid_argument (or use your project's preferred error type)
indicating "clique_cousin_minhash_k must be > 0"; otherwise assign
clique_cousin_minhash_k_ = v. This enforces the contract at the boundary and
prevents downstream empty-sketch/out-of-bounds issues.
---
Outside diff comments:
In `@cpp/src/mip_heuristics/diversity/diversity_manager.cu`:
- Around line 523-603: The root-relaxation handoff must be gated by a new
root_lp_handoffable flag and the primal must be sanitized before marshaling:
compute root_lp_handoffable (start from root_lp_usable) and only set it true for
termination statuses the crossover expects (e.g.,
pdlp_termination_status_t::Optimal, and optionally
pdlp_termination_status_t::TimeLimit only if an LP feasibility check passes);
run clamp_within_var_bounds(lp_optimal_solution, ...) and any
lp_sol.compute_feasibility()/lp_sol.get_feasible() checks before copying/prior
to calling set_root_relaxation_solution_callback; use
lp_result.get_termination_status(),
lp_result.get_primal_solution()/get_dual_solution()/get_reduced_cost(),
clamp_within_var_bounds, and set_root_relaxation_solution_callback to locate
where to implement the flag, the sanitize step, and the tightened conditional.
In `@cpp/src/mip_heuristics/solver.cu`:
- Around line 430-455: The comment about an async B&B publish of a clique_table
is outdated: remove or rewrite the paragraph describing
set_clique_publish_callback/publish_clique_table/ensure_clique_data since the
callback is commented out and B&B actually owns/builds clique_table_ (via
find_initial_cliques(..., &clique_table_, ...)) and passes it directly into
cut_generation; also delete the commented-out callback block referencing
set_clique_publish_callback to avoid misleading future readers and update the
text to state that branch_and_bound_t maintains clique_table_ internally and
supplies it to cut_generation rather than publishing it to
context.problem_ptr->clique_table.
---
Nitpick comments:
In `@benchmarks/linear_programming/cuopt/miplib2017_optima.hpp`:
- Line 73: The function names kBenchmarkOptima() and kBenchmarkInfeasible()
violate the project's snake_case convention; rename these functions to
benchmark_optima() and benchmark_infeasible() (and update their declarations and
definitions accordingly) and update all call sites that reference
kBenchmarkOptima and kBenchmarkInfeasible to the new names; ensure the return
types and const/ref qualifiers remain unchanged and run a build to catch any
remaining references.
In `@cpp/include/cuopt/linear_programming/mip/solver_settings.hpp`:
- Line 132: Add a Doxygen-style comment for the cut-settings group (or
immediately above the member) explaining the semantics of zero_half_cuts:
zero_half_cuts uses the same convention as other cut settings where -1 =
automatic, 0 = disabled, >0 = enabled (positive values control
aggressiveness/limit), and note that enabling zero-half cuts (value != 0)
triggers asynchronous clique-table preparation when thread counts are sufficient
(same behavior as clique_cuts); reference the member name zero_half_cuts and the
related clique_cuts setting in the comment so maintainers can locate the linked
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: aaaebcac-3cc5-4505-8de2-edaa79f781e5
📒 Files selected for processing (14)
benchmarks/linear_programming/cuopt/miplib2017_optima.hppcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/diversity/lns/rins.cucpp/src/mip_heuristics/diversity/recombiners/sub_mip.cuhcpp/src/mip_heuristics/solver.cucpp/src/utilities/manual_cuda_graph.cuhcpp/src/utilities/omp_helpers.hppcpp/tests/mip/cuts_test.cuskills/cuopt-developer/references/contributing.md
💤 Files with no reviewable changes (1)
- cpp/src/utilities/omp_helpers.hpp
akifcorduk
added
improvement
Contributor
Author
|
/ok to test cfb5f4b |
Contributor
Author
|
/ok to test 3edceae |
rgsl888prabhu
approved these changes
Jun 12, 2026
Collaborator
|
/nvskills-ci |
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.
This branch adds zero-half (odd-cycle / odd-wheel) cuts to the set of cuts. The cut generator runs on the same fractional conflict-graph subgraph that clique cuts use: once per cut pass, prepare_fractional_sub_cg() builds the subgraph then Dijkstra finds violated odd cycles on the auxillarry conflict graph (edge weights summing below 0.5), optionally extends them to odd-wheels by adding fully conflicting variables(similar logic to clique extension). Clique cut was refactored to share that subgraph and the same greedy extension helpers, and implied-bound cuts were moved earlier in the pass so the background clique-table thread has more time to finish before we join it.
A-> this PR
B-> main
Gap Closed Metrics:
Top 10 A>B (A-B), gap_closed_pct
Top 10 B>A (B-A), gap_closed_pct
Average and shifted geomean gap closed
A avg=29.648 sgm=8.1513 (shift=1)
B avg=28.924 sgm=7.7679 (shift=1)
Benchmark Results
A Batch 1
total_optimal: 79, avg_mip_gap: 0.3173, geomean_mip_gap: 0.1947, n_low_error: 125
A Batch 2
total_optimal: 77, avg_mip_gap: 0.3175, geomean_mip_gap: 0.1972, n_low_error: 124
B Batch 1
total_optimal: 75, avg_mip_gap: 0.3419, geomean_mip_gap: 0.2062, n_low_error: 119
B Batch2
total_optimal: 75, avg_mip_gap: 0.3126, geomean_mip_gap: 0.1930, n_low_error: 118
On average +3 optimal solutions, -0.4% geomean MIP gap reductions.