Vector length + Farkas diving#1401
Open
nguidotti wants to merge 5 commits into
Open
Conversation
…udo_costs, worker and diving_heuristics. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…derabbit comments. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
📝 WalkthroughWalkthroughThis PR adds templated diving hyper-parameters, implements Farkas and vector-length diving heuristics, extends strategy enums and enablement logic, updates solver settings and worker wiring, and registers new configuration parameters. ChangesDiving Heuristics Configuration and Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/pseudo_costs.hpp (1)
32-37: 🏗️ Heavy liftForward declarations here conflict with the repository header policy.
This header now depends on forward declarations for
branch_and_bound_worker_t/branch_and_bound_stats_t. Please refactor the include graph (for example by splitting strategy-enable utilities into a lighter header) so this header can include concrete dependencies instead of forward-declaring them.As per coding guidelines: "Avoid forward declarations in favor of including headers in C++."
🤖 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/pseudo_costs.hpp` around lines 32 - 37, The forward declarations of branch_and_bound_worker_t and branch_and_bound_stats_t in pseudo_costs.hpp violate the header policy; replace these forward declarations by including the concrete headers that define branch_and_bound_worker_t and branch_and_bound_stats_t (or refactor by extracting the minimal strategy utilities used by pseudo_costs.hpp into a lightweight header that both this file and the full worker/stats headers can include). Update pseudo_costs.hpp to `#include` the new/lightweight header (or the original concrete headers) so the types are available without forward declarations and adjust any include guards or dependencies accordingly.Source: Coding guidelines
🤖 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/include/cuopt/linear_programming/constants.h`:
- Around line 117-130: The constants header is missing the config key for the
parameter defined as farkas_obj_dynamism_tol in diving_hyper_params.hpp; add a
matching string constant (e.g., CUOPT_MIP_HYPER_DIVING_FARKAS_OBJ_DYNAMISM_TOL)
to cpp/include/cuopt/linear_programming/constants.h next to the other
diving-related defines so the config loader can find
"mip_hyper_diving_farkas_obj_dynamism_tol"; ensure the macro name matches the
naming pattern used for other keys (CUOPT_MIP_HYPER_DIVING_*) and the string
matches the parameter name used in diving_hyper_params.hpp.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1754-1757: Guard the division by checking objective magnitudes
before computing obj_dyn: if original_lp_.max_abs_obj_coeff == 0 set
diving_settings.farkas_diving = 0 (all-zero objective -> treat as low dynamism),
else only compute obj_dyn = log10(max/min) when original_lp_.min_abs_obj_coeff >
0 (use a tiny epsilon if you prefer to treat near-zero as zero); if min <= 0
skip the log10 check (leave farkas_diving enabled) to avoid inf/NaN, and keep
the existing comparison against diving_settings.farkas_obj_dynamism_tol when
obj_dyn is valid. Ensure you reference original_lp_.max_abs_obj_coeff,
original_lp_.min_abs_obj_coeff, diving_settings.farkas_diving, and
diving_settings.farkas_obj_dynamism_tol in the fix.
In `@cpp/src/branch_and_bound/diving_heuristics.cpp`:
- Around line 292-299: The Farkas score uses the wrong post-branch distances:
when dir == branch_direction_t::UP the branch bounds fix x_j >=
std::ceil(solution[j]) so compute remaining distance as lp.upper[j] -
std::ceil(solution[j]) (not lp.upper[j] - std::floor(solution[j])), and when dir
== branch_direction_t::DOWN use std::floor(solution[j]) - lp.lower[j] (not
std::ceil(solution[j]) - lp.lower[j]); update the calculation of score (the
block around score, lp.upper, lp.lower, solution[j], f_up, f_down, c, and f_t)
to use these post-branch bounds and keep the infinity fallback for non-finite
bounds.
In `@cpp/src/math_optimization/solver_settings.cu`:
- Line 175: The current option for CUOPT_MIP_HYPER_DIVING_BACKTRACK_LIMIT
exposes a raw upper bound of std::numeric_limits<i_t>::max(), but code
calculating the DFS stack uses (diving_backtrack_limit + 4) and can overflow;
change the option's maximum to a safe cap such as
(std::numeric_limits<i_t>::max() - 4) (or define a named constant like
MAX_SAFE_BACKTRACK = std::numeric_limits<i_t>::max() - 4) and use that instead
for the max value of mip_settings.diving_params.backtrack_limit so adding 4
cannot overflow.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/pseudo_costs.hpp`:
- Around line 32-37: The forward declarations of branch_and_bound_worker_t and
branch_and_bound_stats_t in pseudo_costs.hpp violate the header policy; replace
these forward declarations by including the concrete headers that define
branch_and_bound_worker_t and branch_and_bound_stats_t (or refactor by
extracting the minimal strategy utilities used by pseudo_costs.hpp into a
lightweight header that both this file and the full worker/stats headers can
include). Update pseudo_costs.hpp to `#include` the new/lightweight header (or the
original concrete headers) so the types are available without forward
declarations and adjust any include guards or dependencies accordingly.
🪄 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: b33259e4-7a93-4f0f-9856-e8d5aea35845
📒 Files selected for processing (15)
cpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/diving_hyper_params.hppcpp/include/cuopt/linear_programming/mip/heuristics_hyper_params.hppcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/constants.hppcpp/src/branch_and_bound/diving_heuristics.cppcpp/src/branch_and_bound/diving_heuristics.hppcpp/src/branch_and_bound/pseudo_costs.hppcpp/src/branch_and_bound/worker.hppcpp/src/dual_simplex/presolve.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/solver.cucpp/tests/mip/heuristics_hyper_params_test.cu
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
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
1 participant
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.
Implemented Farkas [1] and vector length [Section 9.2.6, 2] diving heuristics. It includes #1364.
Farkas diving is designed to move the current LP relaxation in direction to a valid Farkas proof. It pushes all variables towards the so-called pseudo solution, in which each variable assumes the best bound with respect to its objective coefficient as the solution value. Generally, this is very optimistic and most of the time, it will violate the constraints of the problem. Yet, if this finds a feasible solution, then it is expected to be very good.
Vector length diving is tailored for set partitioning and set covering. It chooses a rounding that coverst the largest number of constraints with the smallest objective value deterioration.
Benchmark Results
MIPLIB2017, 10min, GH200
All
Vector length diving
Farkas diving
References
[1] J. Witzig and A. Gleixner, “Conflict-Driven Heuristics for Mixed Integer Programming,” Feb. 07, 2019, arXiv: arXiv:1902.02615. doi: 10.48550/arXiv.1902.02615.
[2] T. Achterberg, “Constraint Integer Programming,” PhD, Technischen Universität Berlin, Berlin, 2007. doi: 10.14279/depositonce-1634.
Checklist