[WIP] Reduced cost strenghtening after incumbent update#956
Open
nguidotti wants to merge 14 commits intoNVIDIA:release/26.04from
Open
[WIP] Reduced cost strenghtening after incumbent update#956nguidotti wants to merge 14 commits intoNVIDIA:release/26.04from
nguidotti wants to merge 14 commits intoNVIDIA:release/26.04from
Conversation
…er and worker pool in two files. removed local reduced cost fixing.
nguidotti
added
do not merge
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughReplaces in-place reduced-cost fixing with a root-level tightening flow (reduced-cost fixing + presolve strengthening), adds per-worker worker and worker-pool types for start-bound coordination, changes node bound APIs to accept start bounds and return feasibility, and updates call sites to propagate and broadcast root bound updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mip_heuristics/solver.cu (1)
226-236:⚠️ Potential issue | 🟠 MajorUse one normalization for
reduced_cost_strengthening.Lines 233-236 overwrite the fallback from Lines 226-228, so
-1still resolves to2and values< -1pass through unchanged. If automatic mode is meant to pick the new incumbent-update behavior, it never will; if it is meant to stay at2, the new assignment is dead code.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)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/solver.cu` around lines 226 - 236, The two assignments to branch_and_bound_settings.reduced_cost_strengthening conflict; normalize context.settings.reduced_cost_strengthening once and assign that result. Compute a local rc = context.settings.reduced_cost_strengthening and normalize like: if rc == -1 set rc_normalized = 2; else if rc >= 0 set rc_normalized = rc; else set rc_normalized = 3; then set branch_and_bound_settings.reduced_cost_strengthening = rc_normalized (replace both existing assignments). This uses the unique symbols branch_and_bound_settings.reduced_cost_strengthening and context.settings.reduced_cost_strengthening so the logic is clear and deterministic.
🧹 Nitpick comments (2)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
449-496: Consider clarifying the snapshot-based bounds update pattern.The code copies
original_lp_.lower/upperundermutex_original_lp_, then performs reduced cost fixing on this snapshot, and finally applies updates viaupdate_root_bounds. If bounds are modified by another thread between the copy and the update, those intermediate changes would be overwritten.This appears intentional for correctness (reduced cost fixing based on consistent snapshot), but a brief comment explaining this design would help future maintainers understand the rationale.
🤖 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 449 - 496, The code takes a snapshot of original_lp_.lower and original_lp_.upper under mutex_original_lp_, uses that snapshot for find_reduced_cost_fixings, and then calls update_root_bounds to apply the snapshot-derived changes; add a short explanatory comment near the block that copies original_lp_.lower/upper (around mutex_original_lp_.lock/unlock) clarifying that the snapshot is intentional to ensure reduced-cost fixings are computed from a consistent view and that update_root_bounds may overwrite any intermediate bound changes made by other threads to preserve correctness; reference mutex_original_lp_, original_lp_.lower/upper, find_reduced_cost_fixings, bounds_changed, and update_root_bounds in the comment so future maintainers understand the rationale.cpp/src/branch_and_bound/worker.hpp (1)
144-149: Non-atomic check-and-clear pattern forstart_bounds_updated.The check-and-clear of
start_bounds_updated(lines 146-148) is not atomic. Ifbroadcast_root_bounds_change()sets this flag between the check and the reset, the update notification could be lost for this iteration.Given the use case (triggering bounds recomputation), missing one update is likely acceptable since it will be caught on the next iteration. However, if stronger guarantees are needed, consider using
compare_exchangeor document this as intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/worker.hpp` around lines 144 - 149, The current non-atomic check-and-clear of start_bounds_updated before setting recompute_bounds can race with broadcast_root_bounds_change() and lose an update; make the intent explicit by either converting start_bounds_updated to an std::atomic<bool> and replacing the check-and-clear with an atomic test-and-clear (e.g., compare_exchange or exchange) so the flag is consumed atomically, or if the relaxed “lose-one-update-is-acceptable” behavior is intended, add a clear comment next to start_bounds_updated and this block documenting that choice; refer to start_bounds_updated, recompute_bounds, and broadcast_root_bounds_change() when applying the change.
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Around line 493-494: Update the typo in the log message string "Bound
strenghtening failed when updating the bounds at the root node!" to "Bound
strengthening failed when updating the bounds at the root node!" wherever it
appears; locate the occurrences by searching for the exact message passed to
settings_.log.printf (and similar printf calls) in branch_and_bound.cpp and
replace "strenghtening" with "strengthening" (also update any identical messages
in other branches that set solver_status_ = mip_status_t::NUMERICAL so all
instances are consistent).
- Around line 1756-1765: The call to start_node.value()->check_variable_bounds
reads original_lp_.lower and original_lp_.upper without synchronization, causing
a potential data race with update_root_bounds; fix by acquiring
mutex_original_lp_ (or otherwise synchronizing) before accessing
original_lp_.lower/upper or by copying original_lp_.lower and original_lp_.upper
into local vectors under mutex_original_lp_ and then calling
check_variable_bounds with those copies; update the code around the check in
branch_and_bound.cpp (the block that calls check_variable_bounds on start_node)
to use mutex_original_lp_ or the captured copies to ensure safe concurrent
access.
- Around line 3166-3181: In solve_node_deterministic, reads of
original_lp_.lower and original_lp_.upper (used in calls to
node_ptr->get_variable_bounds and node_ptr->update_branched_variable_bounds) are
racy with update_root_bounds/set_new_solution; protect these accesses by
acquiring mutex_original_lp_ (the same mutex used in update_root_bounds) around
the region that reads original_lp_.lower/upper, or take a local snapshot of
original_lp_.lower and original_lp_.upper under mutex at the start of
solve_node_deterministic and use the snapshot for the subsequent calls so
node_ptr->get_variable_bounds / node_ptr->update_branched_variable_bounds and
assignments to worker.leaf_problem.lower/upper are fed consistent, thread-safe
bounds.
In `@cpp/src/branch_and_bound/deterministic_workers.hpp`:
- Around line 316-326: enqueue_dive_node currently ignores the boolean result of
get_variable_bounds and always detaches and queues the node, which can enqueue
stale/invalid dive nodes; change enqueue_dive_node to check the return value of
get_variable_bounds (called on node with original_lp and filling
entry.resolved_lower/upper) and if it returns false, do not call
node->detach_copy() or push entry to dive_queue (free or leave node unchanged),
otherwise proceed to set entry.node = node->detach_copy() and
dive_queue.push_back(std::move(entry)); ensure you reference dive_queue_entry_t,
detach_copy, and dive_queue when making the check.
In `@cpp/src/branch_and_bound/mip_node.hpp`:
- Around line 104-120: The check_variable_bounds method in mip_node_t
incorrectly rejects ancestor bounds for variables that have been re-branched
deeper in the tree; update check_variable_bounds(branch_var, branch_var_lower,
branch_var_upper) to maintain a seen-variable set (e.g., a
std::unordered_set<int> seen_vars or a bitset keyed by branch_var) while walking
from this node up through parent pointers so that when you encounter an ancestor
with the same branch_var you skip that ancestor's bounds (i.e., ignore outdated
looser bounds) and only validate the first occurrence for each variable; apply
the same seen-variable logic to the analogous ancestor-check block referenced
around lines 165-179 to prevent false pruning after root tightening.
In `@cpp/src/branch_and_bound/worker_pool.hpp`:
- Around line 83-91: broadcast_root_bounds_change() currently only updates
start_bounds_updated for workers with search_strategy == BEST_FIRST; change it
to mark start_bounds_updated = true for any active worker so diving strategies
(PSEUDOCOST_DIVING, LINE_SEARCH_DIVING, GUIDED_DIVING, COEFFICIENT_DIVING) also
receive incumbent-driven root-bound updates. In practice, inside
broadcast_root_bounds_change() keep the is_initialized guard, iterate workers_
and for each worker where workers_[i]->is_active set
workers_[i]->start_bounds_updated = true (remove the BEST_FIRST-only condition);
this ensures all strategies returned by
get_search_strategies()/get_max_workers() get the update.
---
Outside diff comments:
In `@cpp/src/mip_heuristics/solver.cu`:
- Around line 226-236: The two assignments to
branch_and_bound_settings.reduced_cost_strengthening conflict; normalize
context.settings.reduced_cost_strengthening once and assign that result. Compute
a local rc = context.settings.reduced_cost_strengthening and normalize like: if
rc == -1 set rc_normalized = 2; else if rc >= 0 set rc_normalized = rc; else set
rc_normalized = 3; then set branch_and_bound_settings.reduced_cost_strengthening
= rc_normalized (replace both existing assignments). This uses the unique
symbols branch_and_bound_settings.reduced_cost_strengthening and
context.settings.reduced_cost_strengthening so the logic is clear and
deterministic.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 449-496: The code takes a snapshot of original_lp_.lower and
original_lp_.upper under mutex_original_lp_, uses that snapshot for
find_reduced_cost_fixings, and then calls update_root_bounds to apply the
snapshot-derived changes; add a short explanatory comment near the block that
copies original_lp_.lower/upper (around mutex_original_lp_.lock/unlock)
clarifying that the snapshot is intentional to ensure reduced-cost fixings are
computed from a consistent view and that update_root_bounds may overwrite any
intermediate bound changes made by other threads to preserve correctness;
reference mutex_original_lp_, original_lp_.lower/upper,
find_reduced_cost_fixings, bounds_changed, and update_root_bounds in the comment
so future maintainers understand the rationale.
In `@cpp/src/branch_and_bound/worker.hpp`:
- Around line 144-149: The current non-atomic check-and-clear of
start_bounds_updated before setting recompute_bounds can race with
broadcast_root_bounds_change() and lose an update; make the intent explicit by
either converting start_bounds_updated to an std::atomic<bool> and replacing the
check-and-clear with an atomic test-and-clear (e.g., compare_exchange or
exchange) so the flag is consumed atomically, or if the relaxed
“lose-one-update-is-acceptable” behavior is intended, add a clear comment next
to start_bounds_updated and this block documenting that choice; refer to
start_bounds_updated, recompute_bounds, and broadcast_root_bounds_change() when
applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13a663fd-c164-4372-9e91-020f8604d80c
📒 Files selected for processing (10)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/deterministic_workers.hppcpp/src/branch_and_bound/mip_node.hppcpp/src/branch_and_bound/pseudo_costs.hppcpp/src/branch_and_bound/reduced_cost_fixing.hppcpp/src/branch_and_bound/worker.hppcpp/src/branch_and_bound/worker_pool.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/mip_heuristics/solver.cu
…tions. fix incorrect LP problem passed to the coefficient diving.
Contributor
Author
|
/ok to test 1862e36 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp/src/branch_and_bound/reduced_cost_fixing.hpp (1)
32-33: Potential narrowing conversion fromsize_ttoi_t.
reduced_costs.size()returnssize_t, which is assigned toi_t. Ifi_tisint, this could truncate on very large problems. Consider usingstatic_cast<i_t>to make the conversion explicit, or validate that the size fits within the range ofi_t.🔧 Proposed fix
- i_t num_cols_to_check = reduced_costs.size(); // Reduced costs will be smaller than the original - // problem because we have added slacks for cuts + i_t num_cols_to_check = static_cast<i_t>(reduced_costs.size()); // Reduced costs will be smaller than the original + // problem because we have added slacks for cuts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/reduced_cost_fixing.hpp` around lines 32 - 33, The assignment to i_t num_cols_to_check = reduced_costs.size() risks a narrowing conversion from size_t to i_t; change this to an explicit conversion and guard: compute size_t rc_size = reduced_costs.size(), assert or check rc_size fits in the range of i_t, then assign using num_cols_to_check = static_cast<i_t>(rc_size). Update references to num_cols_to_check accordingly and/or handle the overflow case (throw or clamp) so the conversion is explicit and safe.cpp/src/branch_and_bound/worker.hpp (1)
114-128: Consider makingoriginal_lpparameter const.The function only reads
original_lp.lowerandoriginal_lp.upperto copy them intostart_lowerandstart_upper. A const reference would clarify intent and prevent accidental modifications.🔧 Proposed fix
- bool init_best_first(mip_node_t<i_t, f_t>* node, lp_problem_t<i_t, f_t>& original_lp) + bool init_best_first(mip_node_t<i_t, f_t>* node, const lp_problem_t<i_t, f_t>& original_lp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/worker.hpp` around lines 114 - 128, The init_best_first function should take original_lp as a const reference since it only reads original_lp.lower and original_lp.upper; change the signature to init_best_first(mip_node_t<i_t, f_t>* node, const lp_problem_t<i_t, f_t>& original_lp) and update any callers accordingly (ensure mip_node_t::check_variable_bounds still accepts const lower/upper or pass originals unchanged); this clarifies intent and prevents accidental modification of original_lp inside init_best_first.
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Around line 1773-1781: Call the same mutex around reads of original_lp_ before
invoking init_best_first to avoid the race: wrap access to original_lp_ with the
same mutex used in update_root_bounds (mutex_original_lp_) — mirroring the
init_diving pattern — then call worker->init_best_first(start_node.value(),
original_lp_) while holding the lock and release it immediately after; ensure
the lock scope is minimal and only protects reading original_lp_ to match the
concurrency pattern used elsewhere.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/reduced_cost_fixing.hpp`:
- Around line 32-33: The assignment to i_t num_cols_to_check =
reduced_costs.size() risks a narrowing conversion from size_t to i_t; change
this to an explicit conversion and guard: compute size_t rc_size =
reduced_costs.size(), assert or check rc_size fits in the range of i_t, then
assign using num_cols_to_check = static_cast<i_t>(rc_size). Update references to
num_cols_to_check accordingly and/or handle the overflow case (throw or clamp)
so the conversion is explicit and safe.
In `@cpp/src/branch_and_bound/worker.hpp`:
- Around line 114-128: The init_best_first function should take original_lp as a
const reference since it only reads original_lp.lower and original_lp.upper;
change the signature to init_best_first(mip_node_t<i_t, f_t>* node, const
lp_problem_t<i_t, f_t>& original_lp) and update any callers accordingly (ensure
mip_node_t::check_variable_bounds still accepts const lower/upper or pass
originals unchanged); this clarifies intent and prevents accidental modification
of original_lp inside init_best_first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df393de4-b16d-4e5a-af7a-d5c4777fbe32
📒 Files selected for processing (5)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/reduced_cost_fixing.hppcpp/src/branch_and_bound/worker.hppcpp/src/branch_and_bound/worker_pool.hppcpp/src/dual_simplex/simplex_solver_settings.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/dual_simplex/simplex_solver_settings.hpp
Contributor
Author
|
/ok to test bd1ad23 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
3137-3156:⚠️ Potential issue | 🔴 CriticalProtect deterministic root-bound reads with
mutex_original_lp_.The FIXME is still real: Line 3146 and Line 3152 read
original_lp_.lower/upperwithout synchronization, while other threads update those vectors undermutex_original_lp_. This is a data race in deterministic solve.As per coding guidelines, "Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state."🔧 Minimal fix (snapshot bounds under mutex, then use snapshot)
+ std::vector<f_t> start_lower; + std::vector<f_t> start_upper; + mutex_original_lp_.lock(); + start_lower = original_lp_.lower; + start_upper = original_lp_.upper; + mutex_original_lp_.unlock(); + if (worker.recompute_bounds_and_basis) { - feasible = node_ptr->get_variable_bounds(original_lp_.lower, - original_lp_.upper, + feasible = node_ptr->get_variable_bounds(start_lower, + start_upper, worker.leaf_problem.lower, worker.leaf_problem.upper, worker.bounds_changed); } else { - feasible = node_ptr->update_branched_variable_bounds(original_lp_.lower, - original_lp_.upper, + feasible = node_ptr->update_branched_variable_bounds(start_lower, + start_upper, worker.leaf_problem.lower, worker.leaf_problem.upper, worker.bounds_changed); }🤖 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 3137 - 3156, The code in solve_node_deterministic currently reads original_lp_.lower and original_lp_.upper without synchronization causing a data race with updates performed under mutex_original_lp_ (e.g., in update_root_bounds called from set_new_solution); to fix, inside solve_node_deterministic acquire mutex_original_lp_ (or better: take a local snapshot) before any access to original_lp_.lower/upper, copy them into local vectors (e.g., snapshot_lower/snapshot_upper), release the mutex, and then use snapshot_lower/snapshot_upper in the calls to node_ptr->get_variable_bounds and node_ptr->update_branched_variable_bounds instead of original_lp_.lower/upper so no unsynchronized reads occur.
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Line 2650: Deterministic mode currently skips reduced-cost fixing (comment at
branch_and_bound.cpp) so new incumbents there don't call
root_reduced_cost_fixing(...); modify the incumbent-update path used in
deterministic runs to invoke root_reduced_cost_fixing(...) with the same
parameters as the non-deterministic branch (ensure you call it from the
deterministic incumbent-update routine that updates the incumbent and root
bounds), preserve any guard checks or flags around reduced-cost fixing, and
mirror any logging or error handling present in the existing non-deterministic
invocation to keep behavior consistent.
- Around line 3762-3772: The calls to node_ptr->get_variable_bounds and
node_ptr->update_branched_variable_bounds ignore their boolean feasibility
return values; change the deterministic-dive logic to check those returns and
abort the dive when they indicate infeasibility. Specifically, after calling
node_ptr->get_variable_bounds(...) and
node_ptr->update_branched_variable_bounds(...), test the returned bool and if
false stop the current dive flow (e.g., set the dive/leaf infeasible flag,
undo/clean any partial state for worker.leaf_problem and worker.bounds_changed,
and return or jump to the existing infeasible-handling path used elsewhere in
this routine). Ensure you reference the same variables (worker.dive_lower,
worker.dive_upper, worker.leaf_problem.lower/upper, worker.bounds_changed) so
the existing cleanup and logging are used consistently.
---
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3137-3156: The code in solve_node_deterministic currently reads
original_lp_.lower and original_lp_.upper without synchronization causing a data
race with updates performed under mutex_original_lp_ (e.g., in
update_root_bounds called from set_new_solution); to fix, inside
solve_node_deterministic acquire mutex_original_lp_ (or better: take a local
snapshot) before any access to original_lp_.lower/upper, copy them into local
vectors (e.g., snapshot_lower/snapshot_upper), release the mutex, and then use
snapshot_lower/snapshot_upper in the calls to node_ptr->get_variable_bounds and
node_ptr->update_branched_variable_bounds instead of original_lp_.lower/upper so
no unsynchronized reads occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0a41f46-5668-44fa-b9d1-229adfafd6f4
📒 Files selected for processing (3)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/deterministic_workers.hpp
nguidotti
force-pushed
the
reduced-cost-fixing
branch
from
6114f7d to
e5e7d26
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
3769-3780:⚠️ Potential issue | 🟠 MajorHandle infeasible bound reconstruction in deterministic dive.
The calls to
node_ptr->get_variable_boundsandnode_ptr->update_branched_variable_boundsreturn a boolean indicating feasibility, but this return value is ignored. After root tightening or path conflicts, these functions may returnfalse, indicating the node's bounds have become infeasible.Continuing the dive with invalid bounds wastes computation and could produce incorrect results.
🔧 Proposed fix to check feasibility
+ bool bounds_feasible = true; if (worker.recompute_bounds_and_basis) { - node_ptr->get_variable_bounds(worker.dive_lower, - worker.dive_upper, - worker.leaf_problem.lower, - worker.leaf_problem.upper, - worker.bounds_changed); + bounds_feasible = node_ptr->get_variable_bounds(worker.dive_lower, + worker.dive_upper, + worker.leaf_problem.lower, + worker.leaf_problem.upper, + worker.bounds_changed); } else { - node_ptr->update_branched_variable_bounds(worker.dive_lower, - worker.dive_upper, - worker.leaf_problem.lower, - worker.leaf_problem.upper, - worker.bounds_changed); + bounds_feasible = node_ptr->update_branched_variable_bounds(worker.dive_lower, + worker.dive_upper, + worker.leaf_problem.lower, + worker.leaf_problem.upper, + worker.bounds_changed); + } + if (!bounds_feasible) { + worker.recompute_bounds_and_basis = true; + continue; }As per coding guidelines, "Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results."
🤖 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 3769 - 3780, The code ignores the boolean return from node_ptr->get_variable_bounds and node_ptr->update_branched_variable_bounds which can indicate infeasible reconstructed bounds; check the returned bool and if it is false, abort the deterministic dive: set the dive-abort flag used by this routine (e.g., worker.dive_aborted or the appropriate control flag for the caller) and exit the dive processing early (return or break out) to avoid continuing with invalid bounds; update the branch_and_bound logic around worker.recompute_bounds_and_basis, node_ptr, worker.dive_lower, worker.dive_upper, worker.leaf_problem.lower/upper, and worker.bounds_changed to handle the false case consistently.
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2658-2658: Deterministic mode still bypasses reduced-cost fixing on incumbent updates.The TODO comment confirms that reduced-cost fixing is not yet enabled for deterministic mode. When new incumbents are found in deterministic execution, they don't trigger
root_reduced_cost_fixing, potentially missing bound tightening opportunities that the non-deterministic path benefits from.This is noted as a known limitation. Consider tracking this as a follow-up task if the feature parity is desired.
Would you like me to draft a patch that invokes
root_reduced_cost_fixingduring the deterministic sync callback when the incumbent is updated, or open an issue to track this enhancement?🤖 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 2658, Deterministic mode currently skips reduced-cost fixing on incumbent updates; update the deterministic sync callback that handles new incumbents to invoke root_reduced_cost_fixing just like the non-deterministic path does. Locate the code that updates the incumbent in deterministic execution (the deterministic sync/callback handler) and add a call to root_reduced_cost_fixing with the same parameters/flags used by the non-deterministic branch, ensuring any required locks or solver state checks are performed before calling and preserving existing error/return handling; if there are concurrency or ordering differences, add the minimal synchronization used elsewhere when calling root_reduced_cost_fixing so behavior matches the non-deterministic flow, and consider leaving a TODO or opening an issue if full parity requires additional changes.
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Around line 3145-3165: The code reads original_lp_.lower and
original_lp_.upper without synchronization in solve_node_deterministic, causing
a data race with update_root_bounds/set_new_solution which holds
mutex_original_lp_; fix by acquiring mutex_original_lp_ before any access to
original_lp_.lower/upper (or alternatively take a locked snapshot of the bounds
at the start of solve_node_deterministic) and then pass that protected snapshot
to the calls to node_ptr->get_variable_bounds and
node_ptr->update_branched_variable_bounds (retain existing logic for
worker.recompute_bounds_and_basis and worker.leaf_problem.* but use the
synchronized snapshot instead of directly accessing original_lp_).
In `@cpp/src/branch_and_bound/reduced_cost_fixing.hpp`:
- Around line 46-69: The code computes reduced_cost_upper_bound and
reduced_cost_lower_bound but only applies updates when var_types[j] ==
variable_type_t::INTEGER, thereby skipping BINARY variables; change the
integer-check to include binary variables (e.g., treat is_integer =
(var_types[j] == variable_type_t::INTEGER || var_types[j] ==
variable_type_t::BINARY)) and use that single flag throughout the loop to decide
when to floor/ceil and when to update upper_bounds[j], lower_bounds[j],
bounds_changed[j], and increment num_improved/num_fixed; ensure comparisons that
detect fixed variables use that is_integer flag and the existing reduced_cost_*
variables, and avoid recomputing the check multiple times by computing
is_integer once per j.
---
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3769-3780: The code ignores the boolean return from
node_ptr->get_variable_bounds and node_ptr->update_branched_variable_bounds
which can indicate infeasible reconstructed bounds; check the returned bool and
if it is false, abort the deterministic dive: set the dive-abort flag used by
this routine (e.g., worker.dive_aborted or the appropriate control flag for the
caller) and exit the dive processing early (return or break out) to avoid
continuing with invalid bounds; update the branch_and_bound logic around
worker.recompute_bounds_and_basis, node_ptr, worker.dive_lower,
worker.dive_upper, worker.leaf_problem.lower/upper, and worker.bounds_changed to
handle the false case consistently.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Line 2658: Deterministic mode currently skips reduced-cost fixing on incumbent
updates; update the deterministic sync callback that handles new incumbents to
invoke root_reduced_cost_fixing just like the non-deterministic path does.
Locate the code that updates the incumbent in deterministic execution (the
deterministic sync/callback handler) and add a call to root_reduced_cost_fixing
with the same parameters/flags used by the non-deterministic branch, ensuring
any required locks or solver state checks are performed before calling and
preserving existing error/return handling; if there are concurrency or ordering
differences, add the minimal synchronization used elsewhere when calling
root_reduced_cost_fixing so behavior matches the non-deterministic flow, and
consider leaving a TODO or opening an issue if full parity requires additional
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23cb560b-2c67-4a4e-9c29-9380bc9ea490
📒 Files selected for processing (2)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/reduced_cost_fixing.hpp
Contributor
Author
|
/ok to test 00576e6 |
Contributor
Author
|
/ok to test e591c34 |
Contributor
Author
|
/ok to test 5d8eebf |
Contributor
Author
|
/ok to test 51a7d48 |
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.
In this PR, we apply reduced cost strengthening to the root node every time the incumbent is updated. This may improve the variable bounds and fix variables to their bounds globally.
This also renamed the files related with the B&B workers to better suit the new file structure.
Pending: MIPLIB benchmark and testing
Checklist