Skip to content

Generate Gomory cuts at the nodes and add them into the cut pool. - #1684

Open
chris-maes wants to merge 3 commits into
NVIDIA:mainfrom
chris-maes:gomory_nodes
Open

Generate Gomory cuts at the nodes and add them into the cut pool. #1684
chris-maes wants to merge 3 commits into
NVIDIA:mainfrom
chris-maes:gomory_nodes

Conversation

@chris-maes

Copy link
Copy Markdown
Contributor

This PR generates gomory cuts at the nodes. These cuts are added to the cut pool. These cuts can then be used after a restart.

@chris-maes
chris-maes requested a review from a team as a code owner August 6, 2026 14:54
@copy-pr-bot

copy-pr-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds MIP parameters for node Gomory cuts and branch-and-bound restarts. It introduces shared cut-pool handling, pure-binary node cuts, tree progress accounting, restart heuristics, and restart-capable worker and solve-loop rebuilding.

Changes

MIP search enhancements

Layer / File(s) Summary
MIP search settings
cpp/include/cuopt/..., cpp/src/dual_simplex/..., cpp/src/math_optimization/..., cpp/src/mip_heuristics/...
Adds node_cuts and max_restarts settings, registers their valid ranges and defaults, and forwards them to branch-and-bound.
Shared node-cut generation
cpp/src/cuts/..., cpp/src/branch_and_bound/branch_and_bound.*
Adds synchronized shared cut-pool updates, node Gomory cut generation, cut verification, pure-binary detection, and conditional cut-pass generation.
Restart state and tree accounting
cpp/src/branch_and_bound/branch_and_bound.hpp, cpp/src/branch_and_bound/mip_node.hpp, cpp/src/branch_and_bound/node_queue.hpp, cpp/src/branch_and_bound/worker*.hpp
Adds restart status and signaling, tree progress counters, queue and tree cleanup, cumulative restart statistics, and re-entrant worker-pool initialization.
Restart heuristic and solve loop
cpp/src/branch_and_bound/branch_and_bound.cpp, cpp/src/branch_and_bound/branch_and_bound.hpp
Estimates tree growth and gap reduction, triggers restarts, rebuilds the root and workers, reuses cuts and pseudocosts, and repeats the solve loop while restart is requested.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested labels: non-breaking, improvement

Suggested reviewers: ramakrishnap-nv, bubullzz, hlinsen

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: generating Gomory cuts at search nodes and adding them to the cut pool.
Description check ✅ Passed The description accurately explains node Gomory cut generation, cut-pool storage, and reuse after restarts.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 2
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch gomory_nodes
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (3)
cpp/src/cuts/cuts.cpp (1)

3496-3512: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

start_time is unused, and node generation has no time budget.

generate_node_cuts accepts start_time but never uses it. generate_gomory_cuts iterates over every basic row and performs tableau solves, and this now runs at every best-first node. Add a time-limit or work-limit check that uses start_time, or drop the parameter to make the absence of a budget explicit. Also remove the two extra blank lines at Lines 3510-3512.

🤖 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/cuts/cuts.cpp` around lines 3496 - 3512, Update generate_node_cuts to
either enforce a time or work limit using its start_time parameter while
generating Gomory cuts, or remove start_time from the function signature and
call sites to make the unbounded behavior explicit. Also remove the two extra
blank lines after the function body.
cpp/src/cuts/cuts.hpp (1)

321-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document that only add_cut is serialized.

mutex_ guards add_cut only. pool_size, score_cuts, get_best_cuts, check_for_duplicate_cuts, and verify_solution read the same storage without the lock. Extend the comment at Line 352 to state that all other members require external serialization, so future callers do not assume the pool is fully thread-safe. See the related finding on the unlocked pool_size() reads in cpp/src/cuts/cuts.cpp.

Also applies to: 351-354

🤖 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/cuts/cuts.hpp` around lines 321 - 326, Extend the thread-safety
documentation near verify_solution and the related pool APIs to state that
mutex_ serializes only add_cut; callers must externally serialize pool_size,
score_cuts, get_best_cuts, check_for_duplicate_cuts, verify_solution, and other
pool accesses.
cpp/src/branch_and_bound/branch_and_bound.cpp (1)

1565-1614: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Make Arow const in the node-cut Gomory path. generate_node_cuts through generate_gomory_cuts, generate_base_equality, and substitute_slacks only reads Arow. Use const csr_matrix_t<i_t, f_t>& to enforce this contract.

🤖 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/branch_and_bound/branch_and_bound.cpp` around lines 1565 - 1614,
Update the node-cut Gomory call chain rooted at generate_node_cuts so Arow is
passed as const csr_matrix_t<i_t, f_t>& through generate_gomory_cuts,
generate_base_equality, and substitute_slacks. Preserve the existing read-only
behavior and adjust any matching declarations or definitions consistently.
🤖 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 2797-2814: The shared global cut pool used by the root and node
cut-generation flow grows indefinitely because cut_pool_t::drop_cuts is
ineffective. Add a bounded-size or age-based eviction policy for node-generated
cuts, and apply it before restart re-separation consumes global_cut_pool_, while
preserving cuts needed for final-solution verification.
- Around line 2561-2563: Change tree_size_estimate and the restart threshold
calculation to use f_t throughout, including intermediate arithmetic, and clamp
each floating-point result to the valid i_t range before narrowing. Update the
associated debug log format in the restart-check code to use %.0f for the
floating-point estimate, including the later logging location.
- Around line 3091-3104: Handle every non-OPTIMAL result from
solve_linear_program_with_advanced_basis in the restarted-root path around
restart_root_lp_status: map each status using the same handling as the initial
root solve, set solver_status_, call set_final_solution, and return before
re-separation or tree rebuilding; only recompute root_objective_ and continue
when the status is OPTIMAL.

In `@cpp/src/branch_and_bound/branch_and_bound.hpp`:
- Around line 247-250: Resolve the unused restart_concurrent_halt_ flag by
either consuming it in the branch-and-bound restart-stop path to signal worker
termination, or removing the member and its assignments/resets while relying on
node_concurrent_halt_ and solver_status_. Ensure the chosen approach preserves
restart behavior and leaves no dead writes.

In `@cpp/src/cuts/cuts.cpp`:
- Around line 3501-3507: Replace the pool_size_before/pool_size_after
differencing around generate_gomory_cuts in cpp/src/cuts/cuts.cpp:3501-3507 with
a count returned by add_cut or a mutex-protected appended-cut counter, and use
that value to update node_cuts_added_. In cpp/src/cuts/cuts.hpp:351-354, update
the cut_pool_t thread-safety comment to state that only add_cut is thread-safe
and that pool_size, score_cuts, get_best_cuts, check_for_duplicate_cuts, and
verify_solution require external serialization.
- Around line 1177-1222: Update cut_pool_t::verify_solution to validate that x
contains at least original_vars_ entries before iterating over stored cuts and
indexing x[j]. If the vector is too short, log an appropriate failure and return
early without accessing x; preserve the existing verification behavior for
sufficiently sized solution vectors.

---

Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1565-1614: Update the node-cut Gomory call chain rooted at
generate_node_cuts so Arow is passed as const csr_matrix_t<i_t, f_t>& through
generate_gomory_cuts, generate_base_equality, and substitute_slacks. Preserve
the existing read-only behavior and adjust any matching declarations or
definitions consistently.

In `@cpp/src/cuts/cuts.cpp`:
- Around line 3496-3512: Update generate_node_cuts to either enforce a time or
work limit using its start_time parameter while generating Gomory cuts, or
remove start_time from the function signature and call sites to make the
unbounded behavior explicit. Also remove the two extra blank lines after the
function body.

In `@cpp/src/cuts/cuts.hpp`:
- Around line 321-326: Extend the thread-safety documentation near
verify_solution and the related pool APIs to state that mutex_ serializes only
add_cut; callers must externally serialize pool_size, score_cuts, get_best_cuts,
check_for_duplicate_cuts, verify_solution, and other pool accesses.
🪄 Autofix

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: 5d3161fa-a2d9-48c2-8b3a-de7ec6a7b5a4

📥 Commits

Reviewing files that changed from the base of the PR and between 07dddec and b41059a.

📒 Files selected for processing (13)
  • cpp/include/cuopt/mathematical_optimization/constants.h
  • cpp/include/cuopt/mathematical_optimization/mip/solver_settings.hpp
  • cpp/src/branch_and_bound/branch_and_bound.cpp
  • cpp/src/branch_and_bound/branch_and_bound.hpp
  • cpp/src/branch_and_bound/mip_node.hpp
  • cpp/src/branch_and_bound/node_queue.hpp
  • cpp/src/branch_and_bound/worker.hpp
  • cpp/src/branch_and_bound/worker_pool.hpp
  • cpp/src/cuts/cuts.cpp
  • cpp/src/cuts/cuts.hpp
  • cpp/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/math_optimization/solver_settings.cu
  • cpp/src/mip_heuristics/solver.cu

Comment on lines +2561 to +2563
i_t tree_size_estimate =
exploration_stats_.restart_nodes_at_last_check +
nodes_since_last_check * (1.0 - current_progress) / progress_since_last_check;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

tree_size_estimate can overflow i_t.

progress_since_last_check is floored at 1E-6. The product nodes_since_last_check * (1.0 - current_progress) / progress_since_last_check therefore reaches 1e6 * nodes_since_last_check. With more than about 2148 nodes between checks, the double value exceeds INT_MAX, and the conversion to i_t is undefined behavior. settings_.restart_tree_size_factor * total_nodes can also overflow for large trees.

Compute the estimate and the threshold in f_t, and clamp before any narrowing.

🐛 Proposed fix
-  i_t tree_size_estimate =
-    exploration_stats_.restart_nodes_at_last_check +
-    nodes_since_last_check * (1.0 - current_progress) / progress_since_last_check;
+  const f_t tree_size_estimate =
+    static_cast<f_t>(exploration_stats_.restart_nodes_at_last_check) +
+    static_cast<f_t>(nodes_since_last_check) * (f_t(1.0) - current_progress) /
+      progress_since_last_check;
@@
-  if (gap_reduction < 1.05 &&
-      tree_size_estimate >= settings_.restart_tree_size_factor * total_nodes) {
+  const f_t tree_size_threshold =
+    static_cast<f_t>(settings_.restart_tree_size_factor) * static_cast<f_t>(total_nodes);
+  if (gap_reduction < 1.05 && tree_size_estimate >= tree_size_threshold) {

Update the %d specifier for tree_size_estimate in the debug log to %.0f.

Also applies to: 2578-2579

🤖 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/branch_and_bound/branch_and_bound.cpp` around lines 2561 - 2563,
Change tree_size_estimate and the restart threshold calculation to use f_t
throughout, including intermediate arithmetic, and clamp each floating-point
result to the valid i_t range before narrowing. Update the associated debug log
format in the restart-check code to use %.0f for the floating-point estimate,
including the later logging location.

Comment on lines +2797 to +2814
// Single shared global cut pool + generator. Both the root cut passes (below) and the per-node
// cut passes (solve_node_lp) append to global_cut_pool_ through this generator's member cut_pool_,
// so the optimal-solution verification in set_final_solution sees every generated cut. Sized with
// the original variable count so add_cut's index guard matches node cuts, which reduce to the
// original variable space.
global_cut_pool_.emplace(original_lp_.num_cols, settings_);
cut_generation_.emplace(*global_cut_pool_,
original_lp_,
settings_,
Arow_,
new_slacks_,
var_types_,
original_problem_,
probing_implied_bound_,
clique_table_,
clique_signal);
cut_pool_t<i_t, f_t>& cut_pool = *global_cut_pool_;
cut_generation_t<i_t, f_t>& cut_generation = *cut_generation_;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

The shared cut pool grows without a bound across the whole tree.

Every best-first node now appends Gomory cuts to global_cut_pool_, and cut_pool_t::drop_cuts is still a no-op. On a long best-first search the pool grows with the node count. score_cuts at each re-separation is quadratic in the pool size because cut_orthogonality compares each candidate against the selected set, and check_for_duplicate_cuts builds a full CSC copy of the pool.

Add a cap on the pool size for node cuts, or an age-based eviction, before the restart re-separation reads the pool.

🤖 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/branch_and_bound/branch_and_bound.cpp` around lines 2797 - 2814, The
shared global cut pool used by the root and node cut-generation flow grows
indefinitely because cut_pool_t::drop_cuts is ineffective. Add a bounded-size or
age-based eviction policy for node-generated cuts, and apply it before restart
re-separation consumes global_cut_pool_, while preserving cuts needed for
final-solution verification.

Comment on lines +3091 to +3104
lp_settings.concurrent_halt = NULL;
lp_status_t restart_root_lp_status = solve_linear_program_with_advanced_basis(
original_lp_,
exploration_stats_.start_time,
lp_settings,
root_relax_soln_,
basis_update,
basic_list,
nonbasic_list,
root_vstatus_,
edge_norms_);
if (restart_root_lp_status == lp_status_t::OPTIMAL) {
root_objective_ = compute_objective(original_lp_, root_relax_soln_.x);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

The restarted root LP status is ignored.

restart_root_lp_status updates root_objective_ only when the status is OPTIMAL. For TIME_LIMIT, WORK_LIMIT, NUMERICAL_ISSUES, or INFEASIBLE, the code continues into re-separation and rebuilds the tree from a stale root_relax_soln_. The solver then reports a result derived from an LP solution that does not match the current bounds.

Handle the non-optimal statuses the same way the initial root solve does: set the matching solver_status_, call set_final_solution, and return.

🐛 Proposed handling
       if (restart_root_lp_status == lp_status_t::OPTIMAL) {
         root_objective_ = compute_objective(original_lp_, root_relax_soln_.x);
+      } else {
+        settings_.log.printf("Restart root LP did not solve to optimality (%s). Stopping.\n",
+                             lp_status_to_string(restart_root_lp_status).c_str());
+        solver_status_ = restart_root_lp_status == lp_status_t::TIME_LIMIT
+                           ? mip_status_t::TIME_LIMIT
+                           : (restart_root_lp_status == lp_status_t::WORK_LIMIT
+                                ? mip_status_t::WORK_LIMIT
+                                : mip_status_t::NUMERICAL);
+        is_running_ = false;
+        set_final_solution(solution, root_objective_);
+        signal_extend_cliques_.store(true, std::memory_order_release);
+#pragma omp taskwait depend(in : *clique_signal)
+        return solver_status_;
       }
🤖 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/branch_and_bound/branch_and_bound.cpp` around lines 3091 - 3104,
Handle every non-OPTIMAL result from solve_linear_program_with_advanced_basis in
the restarted-root path around restart_root_lp_status: map each status using the
same handling as the initial root solve, set solver_status_, call
set_final_solution, and return before re-separation or tree rebuilding; only
recompute root_objective_ and continue when the status is OPTIMAL.

Comment on lines +247 to +250
// Set to 1 to signal all B&B workers to stop so the tree can be restarted. Owned here (unlike the
// reference PR which threads a pointer through the constructor) since the B&B taskgroup is the only
// consumer.
std::atomic<int> restart_concurrent_halt_{0};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for any reader of restart_concurrent_halt_.
rg -nP -C3 '\brestart_concurrent_halt_\b' cpp

Repository: NVIDIA/cuopt

Length of output: 2125


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- relevant declarations and uses ---'
rg -nP -C5 '\b(node_concurrent_halt_|restart_concurrent_halt_|concurrent_halt|solver_status_)\b' cpp/src/branch_and_bound/branch_and_bound.hpp cpp/src/branch_and_bound/branch_and_bound.cpp
printf '%s\n' '--- taskgroup and worker setup ---'
rg -nP -C8 'taskgroup|concurrent_halt|node_concurrent_halt_' cpp/src/branch_and_bound/branch_and_bound.cpp

Repository: NVIDIA/cuopt

Length of output: 49065


Remove or consume restart_concurrent_halt_.

The flag is only assigned and reset. No code reads it. Wire it into the restart-stop path, or remove it and rely on node_concurrent_halt_ and solver_status_.

🤖 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/branch_and_bound/branch_and_bound.hpp` around lines 247 - 250,
Resolve the unused restart_concurrent_halt_ flag by either consuming it in the
branch-and-bound restart-stop path to signal worker termination, or removing the
member and its assignments/resets while relying on node_concurrent_halt_ and
solver_status_. Ensure the chosen approach preserves restart behavior and leaves
no dead writes.

Comment thread cpp/src/cuts/cuts.cpp
Comment on lines +1177 to +1222
template <typename i_t, typename f_t>
i_t cut_pool_t<i_t, f_t>::verify_solution(const std::vector<f_t>& x, f_t tolerance) const
{
i_t num_violated = 0;
f_t max_violation = 0.0;
const i_t num_cuts = cut_storage_.m;
for (i_t row = 0; row < num_cuts; row++) {
const i_t row_start = cut_storage_.row_start[row];
const i_t row_end = cut_storage_.row_start[row + 1];
f_t cut_x = 0.0;
for (i_t p = row_start; p < row_end; p++) {
const i_t j = cut_storage_.j[p];
const f_t cut_coeff = cut_storage_.x[p];
cut_x += cut_coeff * x[j];
}
// Cut is cut'*x >= rhs, so violation is rhs - cut'*x (positive means the solution violates it).
const f_t violation = rhs_storage_[row] - cut_x;
if (violation > tolerance) {
num_violated++;
max_violation = std::max(max_violation, violation);
settings_.log.printf(
"Cut pool verification: cut %d (type %d) violated by optimal solution: cut'x=%.10e < "
"rhs=%.10e (violation %.3e > tol %.1e)\n",
row,
static_cast<int>(cut_type_[row]),
cut_x,
rhs_storage_[row],
violation,
tolerance);
}
}
if (num_violated > 0) {
settings_.log.printf(
"Cut pool verification FAILED: %d of %d cuts violated by the optimal solution (max violation "
"%.3e). Some generated cut is not globally valid.\n",
num_violated,
num_cuts,
max_violation);
} else {
settings_.log.printf(
"Cut pool verification passed: optimal solution satisfies all %d cuts within tol %.1e\n",
num_cuts,
tolerance);
}
return num_violated;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Guard against a short x vector.

verify_solution indexes x[j] for every stored column index. Stored indices are bounded by original_vars_, which is fixed when the pool is constructed. If a caller passes a solution vector shorter than original_vars_, the loop reads out of bounds. Add a size check and return early, so a future caller cannot trigger undefined behavior.

🛡️ Proposed guard
 i_t cut_pool_t<i_t, f_t>::verify_solution(const std::vector<f_t>& x, f_t tolerance) const
 {
+  if (static_cast<i_t>(x.size()) < original_vars_) {
+    settings_.log.printf(
+      "Cut pool verification skipped: solution has %zu entries, pool indexes %d variables\n",
+      x.size(),
+      original_vars_);
+    return 0;
+  }
   i_t num_violated   = 0;
🤖 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/cuts/cuts.cpp` around lines 1177 - 1222, Update
cut_pool_t::verify_solution to validate that x contains at least original_vars_
entries before iterating over stored cuts and indexing x[j]. If the vector is
too short, log an appropriate failure and return early without accessing x;
preserve the existing verification behavior for sufficiently sized solution
vectors.

Comment thread cpp/src/cuts/cuts.cpp
Comment on lines +3501 to +3507
const i_t pool_size_before = cut_pool_.pool_size();
generate_gomory_cuts(
lp, settings, Arow, new_slacks, var_types, basis_update, xstar, basic_list, nonbasic_list);
const i_t pool_size_after = cut_pool_.pool_size();
if (pool_size_after > pool_size_before) {
node_cuts_added_ += pool_size_after - pool_size_before;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Partial locking of cut_pool_t: only add_cut is serialized. mutex_ guards the append in add_cut, but every other member reads the same storage without the lock. Concurrent best-first workers therefore race on non-atomic pool state.

  • cpp/src/cuts/cuts.cpp#L3501-L3507: stop differencing pool_size(). Return the number of appended cuts from add_cut, or increment a counter inside the pool under mutex_, and accumulate that value into node_cuts_added_.
  • cpp/src/cuts/cuts.hpp#L351-L354: extend the comment to state that only add_cut is thread-safe, and that pool_size, score_cuts, get_best_cuts, check_for_duplicate_cuts, and verify_solution require external serialization.
📍 Affects 2 files
  • cpp/src/cuts/cuts.cpp#L3501-L3507 (this comment)
  • cpp/src/cuts/cuts.hpp#L351-L354
🤖 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/cuts/cuts.cpp` around lines 3501 - 3507, Replace the
pool_size_before/pool_size_after differencing around generate_gomory_cuts in
cpp/src/cuts/cuts.cpp:3501-3507 with a count returned by add_cut or a
mutex-protected appended-cut counter, and use that value to update
node_cuts_added_. In cpp/src/cuts/cuts.hpp:351-354, update the cut_pool_t
thread-safety comment to state that only add_cut is thread-safe and that
pool_size, score_cuts, get_best_cuts, check_for_duplicate_cuts, and
verify_solution require external serialization.

i_t num_cpu_threads = -1; // -1 means use default number of threads in branch and bound
i_t symmetry = -1;
i_t max_cut_passes = 10; // number of cut passes to make
i_t node_cuts = 1; // 0 = disable, 1 = enable cut generation at B&B nodes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename it to generate_node_cuts? The others imply a number and this implies a boolean decision.

// RAII swap so the leaf bounds are restored even if generation throws. leaf_problem and
// start_lower/start_upper are worker-owned same-size vectors, so the swap is O(1) and
// thread-local; nothing reads start_lower/start_upper during the synchronous call.
struct bound_swap_guard_t {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this struct out of the function?

}
} bound_swap_guard(worker->leaf_problem, worker->start_lower, worker->start_upper);

cut_generation_->generate_node_cuts(worker->leaf_problem,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we give work units as arguments? Possibly much smaller than the root cut passes?

gap_reduction,
tree_size_estimate);

if (gap_reduction < 1.05 &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put 1.05 as a config or fixed var?

// the root, and the loop rebuilds a fresh, strengthened tree. Strong branching / pseudocosts
// computed once above are kept warm across restarts.
do {
if (settings_.reduced_cost_strengthening >= 2 && upper_bound_.load() < last_upper_bound) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing curly brace seem to be missing.

// so it isolates how many pool cuts were worth re-adding. do_cut_pass records selected cut
// types into the shared cut_info, so the breakdown printed below is cumulative.
f_t reseparate_objective = root_objective_;
for (i_t resep_pass = 0; resep_pass < settings_.max_cut_passes; ++resep_pass) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a separate cut pass loop? Can't we reuse the root one with some changes?

generate_new ? nullptr : &cuts_reused);
if (resep_result.action == cut_pass_action_t::RETURN) {
is_running_ = false;
signal_extend_cliques_.store(true, std::memory_order_release);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need that anymore as clique table generation is done on root cut passes. We don't do that again here.

if (num_fractional == 0) {
set_solution_at_root(solution, cut_info);
is_running_ = false;
signal_extend_cliques_.store(true, std::memory_order_release);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants