Fix root cut CPUFJ solutions being dropped by the GPU heuristics - #1497
Conversation
aliceb-nv
added
bug
|
/ok to test 5906fc1 |
CI Test Summary✅ All 31 test job(s) passed. |
📝 WalkthroughWalkthroughTwo independent small changes: branch_and_bound.cpp adds a solution_callback invocation in the nondeterministic Root Cut CPUFJ improvement path, and solver.cu adds logic to import branch-and-bound's solver-space incumbent into the final returned solution when it is feasible and improves upon the current solution. ChangesB&B Solution Reporting and Incumbent Import
Estimated code review effort: 2 (Simple) | ~10 minutes Related PRs: None identified. Suggested labels: branch-and-bound, mip-heuristics Suggested reviewers: None identified. 🐰 A hop through branches, incumbents in tow, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 2694-2696: The `solution_callback` call in `branch_and_bound.cpp`
is firing too early inside the CPUFJ improvement path. Update this branch to
mirror the gating used in `repair_heuristic_solutions` and
`add_feasible_solution`: only invoke `settings_.solution_callback` after
`improves_incumbent(...)` succeeds and the candidate has passed feasibility
validation through `set_solution_from_heuristics`/`check_guess`. Keep the
callback aligned with a genuine new incumbent so
`dm->rins.new_best_incumbent_callback` is only notified for validated incumbent
improvements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8908fd70-28df-45a2-9675-f2bee5baecfd
📒 Files selected for processing (2)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/mip_heuristics/solver.cu
| if (settings_.solution_callback != nullptr) { | ||
| settings_.solution_callback(user_assignment, obj); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unconditional solution_callback invocation bypasses the incumbent-improvement/feasibility gate used elsewhere.
Every other call site (repair_heuristic_solutions, add_feasible_solution) only invokes settings_.solution_callback after confirming the solution actually improves the incumbent (improves_incumbent(...)) and is feasibility-checked. Here the callback fires unconditionally for any CPUFJ improvement, before set_solution_from_heuristics validates feasibility via check_guess. Since the callback drives dm->rins.new_best_incumbent_callback, which is documented/named to represent a genuine new best incumbent, this can report non-improving or not-yet-validated solutions to RINS/population, polluting downstream state.
🐛 Proposed fix to gate the callback on actual incumbent improvement
} else {
- if (settings_.solution_callback != nullptr) {
- settings_.solution_callback(user_assignment, obj);
- }
+ if (settings_.solution_callback != nullptr && improves_incumbent(obj)) {
+ settings_.solution_callback(user_assignment, obj);
+ }
set_solution_from_heuristics(user_assignment);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (settings_.solution_callback != nullptr) { | |
| settings_.solution_callback(user_assignment, obj); | |
| } | |
| if (settings_.solution_callback != nullptr && improves_incumbent(obj)) { | |
| settings_.solution_callback(user_assignment, obj); | |
| } |
🤖 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 2694 - 2696, The
`solution_callback` call in `branch_and_bound.cpp` is firing too early inside
the CPUFJ improvement path. Update this branch to mirror the gating used in
`repair_heuristic_solutions` and `add_feasible_solution`: only invoke
`settings_.solution_callback` after `improves_incumbent(...)` succeeds and the
candidate has passed feasibility validation through
`set_solution_from_heuristics`/`check_guess`. Keep the callback aligned with a
genuine new incumbent so `dm->rins.new_best_incumbent_callback` is only notified
for validated incumbent improvements.
| sol = dm.run_solver(); | ||
| } // implicit barrier for all tasks created in B&B and heuristics | ||
|
|
||
| if (!context.settings.heuristics_only && branch_and_bound->has_solver_space_incumbent()) { |
There was a problem hiding this comment.
Why is this needed? The callback already registers it on the heuristic thread, right? Is it because of the different thread completion orders?
There was a problem hiding this comment.
It's not, I added this to err on the safe side in case future additions neglect to run the gpu heuristic callback
|
/merge |
2 participants
Root cut CPUFJ solutions were only registered at the B&B layer, which resulted in GPU heuristics assuming the early heuristic incumbent was the best available solution and dropping the root cut solutions.
This is addressed by sending rootcut CPUFJ solutions to gpu heursitic callbacks, and ensuring that the B&B solution is taken at the end of the solve if it is better than the last known incumbent on the gpu heuristic side.
Closes #1488
Description
Issue
Checklist