Skip to content

Conversation

@adityasinghz
Copy link

@adityasinghz adityasinghz commented Nov 24, 2025

Description

Fixed the lower bound calculation during the ramp-up phase in parallel Branch and Bound. Previously, cuOpt was using the root objective as the global lower bound when the heap was empty, which is a pessimistic estimation that doesn't reflect the actual lower bounds calculated by active threads during ramp-up.

Changes Made:

  • Line 1229: Changed from heap_.size() > 0 ? heap_.top()->lower_bound : search_tree.root.lower_bound to get_lower_bound()
  • Line 736: Changed from root_objective_ to get_lower_bound() in ramp-up logging

Why This Fix Works:
The get_lower_bound() function properly considers all sources of lower bound information:

  • lower_bound_ceiling_ (updated during ramp-up when threads encounter numerical issues)
  • Heap top node's lower bound (if heap is not empty)
  • All local_lower_bounds_ from active threads (updated as threads solve nodes during ramp-up)

This ensures the lower bound accurately reflects the work done by active threads during ramp-up, not just the pessimistic root objective.

Example Impact:
During ramp-up, if threads have solved nodes with lower bounds better than the root objective, the global lower bound will now reflect these improved bounds instead of falling back to the root objective.

Issue #445

Checklist

  • I am familiar with the Contributing Guidelines.

  • Testing

    • New or existing tests cover these changes
    • Added tests
    • Created an issue to follow-up
    • NA
  • Documentation

    • The documentation is up to date with these changes
    • Added new documentation
    • NA

During the ramp-up phase in parallel B&B, cuOpt was using the root
objective as the global lower bound when the heap was empty. This is
a pessimistic estimation that doesn't reflect the actual lower bounds
calculated by active threads during ramp-up.

Changes:
- Use get_lower_bound() instead of root_objective_ fallback at end of solve()
- Use get_lower_bound() in ramp-up logging instead of root_objective_

The get_lower_bound() function properly considers:
- lower_bound_ceiling_ (updated during ramp-up)
- Heap top (if heap is not empty)
- All local_lower_bounds_ from active threads

This ensures the lower bound accurately reflects the work done by
active threads during ramp-up, not just the root objective.
@adityasinghz adityasinghz requested a review from a team as a code owner November 24, 2025 15:07
@copy-pr-bot
Copy link

copy-pr-bot bot commented Nov 24, 2025

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.

@chris-maes chris-maes assigned chris-maes and nguidotti and unassigned chris-maes Nov 25, 2025
@chris-maes
Copy link
Contributor

@nguidotti please review

@nguidotti
Copy link
Contributor

Unfortunately, the calculation of the lower bound in the ramp-up phase cannot be solved by a simple code change (note that get_lower_bound() will return root_objective_ anyway since the heap is empty and all entries of local_lower_bounds_ is set to root_objective_). The other code change will produce an incorrect bound if the B&B tree is complete.

So, next time, please verify the effects of your changes before submitting a PR instead of copying the output of a LLM. Thank you.

@nguidotti nguidotti closed this Nov 25, 2025
@adityasinghz
Copy link
Author

I wrote this patch myself, no generated code was used. The PR only replaces root_objective_ with get_lower_bound() in the ramp-up log and the final bound. I appreciate the technical feedback (I agree the change doesn’t solve the ramp-up issue and can mis-handle completed trees), but please keep review comments focused on the code, not personal assumptions. I’ll address the logic concerns in an updated revision. Thank You!

@nguidotti
Copy link
Contributor

nguidotti commented Nov 25, 2025

I am sorry that I assume that you used a LLM (the way that you write seems like a LLM response). In any case, you should check the correctness of your changes by running the MIPLIB dataset and attaching the relevant logs.

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.

3 participants