Add MIP gap to heuristic log during dual-simplex root relaxation#942
Add MIP gap to heuristic log during dual-simplex root relaxation#942anandhkb wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
- Add root_lp_progress_callback to simplex_solver_settings (optional) - Invoke callback in dual_phase2 with current dual objective when inside_mip=1 - B&B: store root LP lower bound via callback, use in report_heuristic - Append 'Gap: X%' to log line when heuristic finds solution during root solve Addresses issue when solver times out during root relaxation: gap can now be computed from last dual feasible solution and heuristic incumbent.
anandhkb
added
improvement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a dual-simplex objective callback to simplex settings, records root-relaxation progress in a new atomic member on branch_and_bound_t, wires the callback into root LP solve paths (phase2 and root solve) to update that atomic, and uses the stored root LP bound in get_lower_bound/report_heuristic and related root-state transitions. 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/dual_simplex/phase2.cpp (1)
3511-3525:⚠️ Potential issue | 🟠 MajorDecouple the root-LP callback from iteration logging.
root_lp_progress_callbackonly fires on log iterations here. If a heuristic finds an incumbent or the root solve exits onTIME_LIMIT/WORK_LIMITbefore the next log tick, the stored root lower bound stays stale or unset, so the newGap: X%output can be wrong or missing. Emit the callback wheneverobjadvances, and seed it once right after the initial root objective is computed.Suggested fix
@@ if (phase == 2) { settings.log.printf("%5d %+.16e %7d %.8e %.2e %.2f\n", iter, compute_user_objective(lp, obj), infeasibility_indices.size(), primal_infeasibility_squared, 0.0, toc(start_time)); } + if (phase == 2 && settings.inside_mip == 1 && settings.root_lp_progress_callback) { + settings.root_lp_progress_callback(compute_user_objective(lp, obj)); + } @@ if ((iter - start_iter) < settings.first_iteration_log || (iter % settings.iteration_log_frequency) == 0) { if (phase == 1 && iter == 1) { settings.log.printf(" Iter Objective Num Inf. Sum Inf. Perturb Time\n"); } settings.log.printf("%5d %+.16e %7d %.8e %.2e %.2f\n", iter, compute_user_objective(lp, obj), infeasibility_indices.size(), primal_infeasibility_squared, sum_perturb, now); - if (phase == 2 && settings.inside_mip == 1 && settings.root_lp_progress_callback) { - settings.root_lp_progress_callback(compute_user_objective(lp, obj)); - } } + if (phase == 2 && settings.inside_mip == 1 && settings.root_lp_progress_callback) { + settings.root_lp_progress_callback(compute_user_objective(lp, obj)); + }Based on learnings, "the dual simplex method is dual-safe ... the current objective value is still a valid lower bound"; this path needs the latest iterate, not just the latest logged one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/dual_simplex/phase2.cpp` around lines 3511 - 3525, The root-LP progress callback is only invoked during logging, leaving the stored root lower bound stale; change logic around root_lp_progress_callback so it is called whenever the objective advances (i.e., whenever compute_user_objective(lp, obj) yields a strictly improved value from the last reported value) and also invoke it once immediately after the initial root objective is computed (right after the first compute_user_objective(lp, obj) for phase==2/root). Update the code paths using settings.root_lp_progress_callback (currently inside the logging block guarded by phase/iter) to compare the current objective to a cached last_reported_root_obj and call settings.root_lp_progress_callback(current_obj) whenever current_obj > last_reported_root_obj (and seed last_reported_root_obj on initial compute); keep existing log behavior unchanged.
🤖 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 1955-1958: Reset root_objective_ alongside
root_lp_current_lower_bound_ during solver initialization/clear so
get_lower_bound() does not return a stale finite value from a prior run; update
the same reset block that currently sets root_lp_current_lower_bound_ = -inf
(and is_running_, solver_status_, exploration_stats_) to also assign
root_objective_ = -inf (or the appropriate sentinel), ensuring functions like
get_lower_bound() and callback-based root bound updates use the new run's
values.
- Around line 323-329: The code currently uses get_lower_bound() (internal
objective space) directly when computing the gap; convert that internal lower
bound into user space before using user_mip_gap. Replace the assignment path
where get_lower_bound() is finite so that you transform the returned value via
the same objective-space conversion used elsewhere (i.e., the conversion applied
to root_lp_current_lower_bound_) and then assign that converted value to
user_lower before calling user_mip_gap<f_t>(user_obj, user_lower); ensure the
conversion function you call is the same one used for storing
root_lp_current_lower_bound_ so both operands to user_mip_gap are in user space.
---
Outside diff comments:
In `@cpp/src/dual_simplex/phase2.cpp`:
- Around line 3511-3525: The root-LP progress callback is only invoked during
logging, leaving the stored root lower bound stale; change logic around
root_lp_progress_callback so it is called whenever the objective advances (i.e.,
whenever compute_user_objective(lp, obj) yields a strictly improved value from
the last reported value) and also invoke it once immediately after the initial
root objective is computed (right after the first compute_user_objective(lp,
obj) for phase==2/root). Update the code paths using
settings.root_lp_progress_callback (currently inside the logging block guarded
by phase/iter) to compare the current objective to a cached
last_reported_root_obj and call settings.root_lp_progress_callback(current_obj)
whenever current_obj > last_reported_root_obj (and seed last_reported_root_obj
on initial compute); keep existing log behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2f452bc-dbda-4867-afe3-e87f9a290b70
📒 Files selected for processing (4)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/simplex_solver_settings.hpp
…et, callback every iteration - Convert get_lower_bound() to user space before user_mip_gap in report_heuristic - Reset root_objective_ at solve start to avoid stale bound on B&B reuse - Invoke root_lp_progress_callback every iteration, not just at log points
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
308-332:⚠️ Potential issue | 🟠 MajorThe fresh callback bound is still ignored during root cut passes.
This only fixes the
!is_running_path. Once root cut passes begin, heuristic incumbents can still be reported while the root LP is in flight, but theis_running_branch keeps formatting the gap fromget_lower_bound(), which will usually resolve to the previousroot_objective_instead of the callback’s current bound. That makes the logged gap stale for solutions found during the cut-pass solve.Possible direction
void branch_and_bound_t<i_t, f_t>::report_heuristic(f_t obj) { + const f_t user_obj = compute_user_objective(original_lp_, obj); + const f_t root_progress_lower = root_lp_current_lower_bound_.load(); + f_t user_lower = std::numeric_limits<f_t>::quiet_NaN(); + + if (exploration_stats_.nodes_unexplored == 0 && std::isfinite(root_progress_lower)) { + user_lower = root_progress_lower; + } else { + const f_t lower = get_lower_bound(); + if (std::isfinite(lower)) { + user_lower = compute_user_objective(original_lp_, lower); + } else if (std::isfinite(root_progress_lower)) { + user_lower = root_progress_lower; + } + } + if (is_running_) { - f_t user_obj = compute_user_objective(original_lp_, obj); - f_t user_lower = compute_user_objective(original_lp_, get_lower_bound()); std::string user_gap = user_mip_gap<f_t>(user_obj, user_lower); settings_.log.printf( "H %+13.6e %+10.6e %s %9.2f\n", @@ } else { - f_t user_obj = compute_user_objective(original_lp_, obj); - f_t lower = get_lower_bound(); - f_t user_lower = std::isfinite(lower) ? compute_user_objective(original_lp_, lower) - : root_lp_current_lower_bound_.load(); std::string gap_str = std::isfinite(user_lower) ? (". Gap: " + user_mip_gap<f_t>(user_obj, user_lower) + "\n") : "\n";Based on learnings,
set_new_solution()can be called from external threads during the root cut pass phase, and dual simplex stays dual-feasible throughout execution, so the callback value is the up-to-date valid bound in that window.🤖 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 308 - 332, The reported gap in report_heuristic(...) can be stale during root cut passes because get_lower_bound() may reflect an older root objective; change the else-branch so user_lower uses the most up-to-date bound by comparing the computed lower (compute_user_objective(original_lp_, lower)) with root_lp_current_lower_bound_.load() and selecting the tighter (larger for minimization) finite value, then format the gap from that chosen user_lower (use existing symbols: report_heuristic, get_lower_bound, compute_user_objective, root_lp_current_lower_bound_.load(), user_mip_gap); this ensures solutions set via set_new_solution() during root cut passes produce a correct gap in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 308-332: The reported gap in report_heuristic(...) can be stale
during root cut passes because get_lower_bound() may reflect an older root
objective; change the else-branch so user_lower uses the most up-to-date bound
by comparing the computed lower (compute_user_objective(original_lp_, lower))
with root_lp_current_lower_bound_.load() and selecting the tighter (larger for
minimization) finite value, then format the gap from that chosen user_lower (use
existing symbols: report_heuristic, get_lower_bound, compute_user_objective,
root_lp_current_lower_bound_.load(), user_mip_gap); this ensures solutions set
via set_new_solution() during root cut passes produce a correct gap in logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9e1e1da-2c62-49cc-8375-7edd7c3b15ea
📒 Files selected for processing (2)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/dual_simplex/phase2.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/dual_simplex/phase2.cpp
…t comment 4) When nodes_explored==0, prefer root_lp_current_lower_bound_ over get_lower_bound() when it provides a tighter bound. Fixes stale gap for heuristics during cut-pass solve.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1961-1965: Reset lower_bound_ceiling_ immediately when preparing
the solver state so that early root-relaxation heuristics (which call
report_heuristic() -> get_lower_bound() while is_running_ == false) do not see a
stale finite value from a previous run; specifically, add an assignment to set
lower_bound_ceiling_ to -inf (or the same sentinel used for
root_lp_current_lower_bound_) alongside the other initializations
(solver_status_, is_running_, root_lp_current_lower_bound_, root_objective_,
exploration_stats_.nodes_unexplored) so that get_lower_bound() will correctly
fall back to root_lp_current_lower_bound_ during the root solve.
- Around line 314-319: The current root-progress promotion only compares
root_lp_current_lower_bound_.load() to user_lower using a '>' check, which is
correct for minimization but wrong for maximization; update the condition in the
block guarded by exploration_stats_.nodes_explored == 0 so it uses the problem's
objective sense: when the objective is minimizing keep the existing
root_progress > user_lower test, but when maximizing use root_progress <
user_lower (or equivalently invert the comparison), using your solver's
objective-sense flag or helper (e.g., an is_minimize()/objective_sense variable)
to decide which comparison to apply before assigning user_lower.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7428bfc2-7174-4675-a89a-01e329dbd914
📒 Files selected for processing (1)
cpp/src/branch_and_bound/branch_and_bound.cpp
| std::string gap_str = std::isfinite(user_lower) | ||
| ? (". Gap: " + user_mip_gap<f_t>(user_obj, user_lower) + "\n") | ||
| : "\n"; | ||
| settings_.log.printf("New solution from primal heuristics. Objective %+.6e. Time %.2f%s", |
There was a problem hiding this comment.
We follow the convention that time is always the last item in the log line.
So I think this should read
"New solution from primal heuristics. Objective %.+.6e. Gap %s. Time %.2f"
| std::function<void(const std::vector<f_t>&, f_t)> node_processed_callback; | ||
| std::function<void()> heuristic_preemption_callback; | ||
| std::function<void(std::vector<f_t>&, std::vector<f_t>&, f_t)> set_simplex_solution_callback; | ||
| std::function<void(f_t)> |
There was a problem hiding this comment.
I think we should call this dual_simplex_objective_callback. It's potentially useful beyond the root lp.
cpp/src/dual_simplex/phase2.cpp
Outdated
| now); | ||
| } | ||
| if (phase == 2 && settings.inside_mip == 1 && settings.root_lp_progress_callback) { | ||
| settings.root_lp_progress_callback(compute_user_objective(lp, obj)); |
There was a problem hiding this comment.
Let's only do this each time we print in dual simplex. So move it up into the if ((iter - start iter) block
And let's store the user_obj and not recompute it twice:
const f_t user_obj = compute_user_objective(lp, obj);
settings.log.printf('...');
if (phase == 2 ...)
settings.dual_simplex_objective_callback(user_obj);
| f_t user_lower = compute_user_objective(original_lp_, get_lower_bound()); | ||
| f_t user_obj = compute_user_objective(original_lp_, obj); | ||
| f_t lower = get_lower_bound(); | ||
| f_t user_lower = std::isfinite(lower) ? compute_user_objective(original_lp_, lower) : -inf; |
There was a problem hiding this comment.
I don't think we need to separate out lower here.
Better to keep this as is:
f_t user_lower = compute_user_objective(original_lp_, get_lower_bound());
| f_t user_lower = std::isfinite(lower) ? compute_user_objective(original_lp_, lower) : -inf; | ||
| if (exploration_stats_.nodes_explored == 0) { | ||
| f_t root_progress = root_lp_current_lower_bound_.load(); | ||
| if (std::isfinite(root_progress) && |
There was a problem hiding this comment.
I think we should simplify this logic. Let's add a
omp_atomic_t solving_root_relaxation;
We can set it to true when we start solving the root and set it to false when we are done with the root.
We should only load the value when solving_root_relaxation is true.
| toc(exploration_stats_.start_time)); | ||
| f_t user_obj = compute_user_objective(original_lp_, obj); | ||
| f_t lower = get_lower_bound(); | ||
| f_t user_lower = std::isfinite(lower) ? compute_user_objective(original_lp_, lower) |
There was a problem hiding this comment.
I'm confused why the logic above is separate. Maybe a better idea to move it down here.
…_ceiling_, time last in heuristic log
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
1962-1966:⚠️ Potential issue | 🟠 MajorDon’t let root-phase heuristic gaps read previous tree state.
get_lower_bound()still consultsnode_queue_andworker_pool_here, but this solve-entry reset only clearslower_bound_ceiling_/root_objective_state. If the solver instance is reused, a stale finite queue/pool bound can win over the live root progress and the new root-gap log becomes wrong. Please gate this path with an explicitsolving_root_relaxation_flag (or clear the tree/pool state first) so root-phase heuristics use onlyroot_lp_current_lower_bound_.Verification: confirm that
node_queue_/worker_pool_are either cleared before any root-phase heuristic can callreport_heuristic(), or that their empty/uninitializedget_lower_bound()paths return+inf. If not, this log path is still reading prior-run state.#!/bin/bash set -euo pipefail echo "Current lower-bound and root-gap logic:" sed -n '291,305p' cpp/src/branch_and_bound/branch_and_bound.cpp echo sed -n '1959,2005p' cpp/src/branch_and_bound/branch_and_bound.cpp echo sed -n '308,340p' cpp/src/branch_and_bound/branch_and_bound.cpp echo echo "Inspect node_queue / worker_pool lower-bound and reset behavior:" fd 'node_queue|worker_pool' cpp | while read -r f; do echo "== $f ==" rg -n -C3 'get_lower_bound\(|clear\(|reset\(|init\(|empty\(' "$f" || true doneAs 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: 311-320, 332-335
🤖 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 1962 - 1966, The root-phase heuristic logging can read stale bounds from node_queue_ or worker_pool_ because reset currently only clears lower_bound_ceiling_, root_lp_current_lower_bound_, and root_objective_; add an explicit solving_root_relaxation_ boolean (set true on entry to root relaxation and false on exit) and modify get_lower_bound() callers (or get_lower_bound() itself) to ignore node_queue_/worker_pool_ when solving_root_relaxation_ is true so only root_lp_current_lower_bound_ is used during root-phase heuristics (alternatively, ensure node_queue_.clear()/worker_pool_.clear() is invoked before any report_heuristic() call); update the reset path in branch_and_bound::solve (and related entry points that set solver_status_ / is_running_) to initialize solving_root_relaxation_, and add a verification check that node_queue_ and worker_pool_.get_lower_bound() return +inf or are empty before reporting root-gap logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1962-1966: The root-phase heuristic logging can read stale bounds
from node_queue_ or worker_pool_ because reset currently only clears
lower_bound_ceiling_, root_lp_current_lower_bound_, and root_objective_; add an
explicit solving_root_relaxation_ boolean (set true on entry to root relaxation
and false on exit) and modify get_lower_bound() callers (or get_lower_bound()
itself) to ignore node_queue_/worker_pool_ when solving_root_relaxation_ is true
so only root_lp_current_lower_bound_ is used during root-phase heuristics
(alternatively, ensure node_queue_.clear()/worker_pool_.clear() is invoked
before any report_heuristic() call); update the reset path in
branch_and_bound::solve (and related entry points that set solver_status_ /
is_running_) to initialize solving_root_relaxation_, and add a verification
check that node_queue_ and worker_pool_.get_lower_bound() return +inf or are
empty before reporting root-gap logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dae1f3fe-1669-4653-8fe3-aaf7df26ed81
📒 Files selected for processing (1)
cpp/src/branch_and_bound/branch_and_bound.cpp
…, unify report_heuristic
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
293-296:⚠️ Potential issue | 🟠 MajorKeep
get_lower_bound()in internal objective space.Line 316 still treats
get_lower_bound()as an internal-space bound and converts it withcompute_user_objective(...), but Lines 293-296 now returnroot_lp_current_lower_bound_directly during the root solve. That same atomic is handled as a user-space value later in Lines 318-323, so root-solve heuristic gaps get double-converted and can be wrong, especially on scaled/maximization models.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 293 - 296, get_lower_bound() must always return the lower bound in internal objective space; currently when solving_root_relaxation_ is true the code returns root_lp_current_lower_bound_ which later is treated as user-space and double-converted. Change the early-return path to return the internal-space value (not converted) and ensure consistency with how root_lp_current_lower_bound_ is interpreted elsewhere: keep get_lower_bound() in internal space, remove any call to compute_user_objective(...) in the root-return branch, and instead apply compute_user_objective(...) only at the single conversion site that produces user-space values (the code around compute_user_objective(...)/lines 318-323) so that root_lp_current_lower_bound_ is not double-converted for scaled/maximization models.
🤖 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 2050-2060: When root_status is TIME_LIMIT or WORK_LIMIT the code
currently calls set_final_solution(solution, -inf) which discards any finite
root bound captured by the callback; update those branches (the checks of
root_status, and the existing uses of solving_root_relaxation_, solver_status_,
and set_final_solution(solution, -inf)) to pass the actual captured root
progress bound (e.g., the root_bound or solution.lower_bound value populated by
the root-callback) instead of -inf so the final solution and returned
solver_status_ preserve the finite root bound for root-gap reporting.
- Line 2128: Several early-return/error paths leave the member flag
solving_root_relaxation_ set true; find the function that sets
solving_root_relaxation_ (the root-phase routine in branch_and_bound.cpp where
that flag is toggled) and ensure every exit resets it. Fix by adding a
single-exit cleanup or an RAII/scope-guard object (e.g., a small guard class or
lambda-deferred action) that sets solving_root_relaxation_ = false on
destruction, or by consolidating returns to a single cleanup label that resets
solving_root_relaxation_. Update all early-return/error paths (including the
ones noted around the current returns) to use the guard/cleanup so the flag is
always cleared.
---
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 293-296: get_lower_bound() must always return the lower bound in
internal objective space; currently when solving_root_relaxation_ is true the
code returns root_lp_current_lower_bound_ which later is treated as user-space
and double-converted. Change the early-return path to return the internal-space
value (not converted) and ensure consistency with how
root_lp_current_lower_bound_ is interpreted elsewhere: keep get_lower_bound() in
internal space, remove any call to compute_user_objective(...) in the
root-return branch, and instead apply compute_user_objective(...) only at the
single conversion site that produces user-space values (the code around
compute_user_objective(...)/lines 318-323) so that root_lp_current_lower_bound_
is not double-converted for scaled/maximization models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6217266d-de81-4546-94b8-1f62950be6fc
📒 Files selected for processing (4)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/simplex_solver_settings.hpp
| if (root_status == lp_status_t::TIME_LIMIT) { | ||
| solver_status_ = mip_status_t::TIME_LIMIT; | ||
| solving_root_relaxation_ = false; | ||
| solver_status_ = mip_status_t::TIME_LIMIT; | ||
| set_final_solution(solution, -inf); | ||
| return solver_status_; | ||
| } | ||
|
|
||
| if (root_status == lp_status_t::WORK_LIMIT) { | ||
| solver_status_ = mip_status_t::WORK_LIMIT; | ||
| solving_root_relaxation_ = false; | ||
| solver_status_ = mip_status_t::WORK_LIMIT; | ||
| set_final_solution(solution, -inf); |
There was a problem hiding this comment.
Don’t discard the captured root bound on initial-root timeout/work-limit.
Lines 2053 and 2060 still call set_final_solution(solution, -inf). If a heuristic incumbent exists when the initial root LP hits TIME_LIMIT or WORK_LIMIT, the final summary and returned solution.lower_bound lose the finite root-progress bound collected by the callback, which defeats the new root-gap reporting path.
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 2050 - 2060, When
root_status is TIME_LIMIT or WORK_LIMIT the code currently calls
set_final_solution(solution, -inf) which discards any finite root bound captured
by the callback; update those branches (the checks of root_status, and the
existing uses of solving_root_relaxation_, solver_status_, and
set_final_solution(solution, -inf)) to pass the actual captured root progress
bound (e.g., the root_bound or solution.lower_bound value populated by the
root-callback) instead of -inf so the final solution and returned solver_status_
preserve the finite root bound for root-gap reporting.
| i_t cut_pool_size = 0; | ||
| for (i_t cut_pass = 0; cut_pass < settings_.max_cut_passes; cut_pass++) { | ||
| if (num_fractional == 0) { | ||
| solving_root_relaxation_ = false; |
There was a problem hiding this comment.
A couple of root-phase exits still leave solving_root_relaxation_ set.
You reset the flag on most exits, but the returns at Line 2222 and Line 2263 still bypass all of these assignments. That leaves the solver object in an inconsistent post-root state if either error path is taken. A small scope guard or single-exit cleanup would make this much harder to miss.
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: 2290-2291, 2314-2315, 2357-2357, 2377-2377
🤖 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 2128, Several
early-return/error paths leave the member flag solving_root_relaxation_ set
true; find the function that sets solving_root_relaxation_ (the root-phase
routine in branch_and_bound.cpp where that flag is toggled) and ensure every
exit resets it. Fix by adding a single-exit cleanup or an RAII/scope-guard
object (e.g., a small guard class or lambda-deferred action) that sets
solving_root_relaxation_ = false on destruction, or by consolidating returns to
a single cleanup label that resets solving_root_relaxation_. Update all
early-return/error paths (including the ones noted around the current returns)
to use the guard/cleanup so the flag is always cleared.
|
/ok to test d7ddec7 |
@anandhkb, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test d7ddec7 |
|
@chris-maes Please advise once more, I think all your previous advises are factored into the latest commit. Thanks. |
2 participants
Addresses issue #938