Complement variables in order to find knapsack cuts#970
Complement variables in order to find knapsack cuts#970chris-maes wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
chris-maes
added
improvement
|
/ok to test daad714 |
📝 WalkthroughWalkthroughUnconditional debug printing for cut-pool types was enabled. Knapsack cut generation was refactored: function renamed, signature extended with an is_complemented vector and cut output, rationalization/approximation utilities were exposed, and complementation logic was added to handle negative coefficients and restore state on exit. 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
Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2223-2227:⚠️ Potential issue | 🟠 MajorGuard cut-pool diagnostics behind debug logging.
Line 2223 forces this block on in all builds (
#if 1), so every cut pass prints unconditionally. In this hot path, that can materially increase runtime/log volume, andprintfon Line 2226 bypasses logger controls.🔧 Suggested fix
-#if 1 +#ifdef CUOPT_LOG_DEBUG cut_pool.print_cutpool_types(); print_cut_types("In LP ", cut_types, settings_); - printf("Cut pool size: %d\n", cut_pool.pool_size()); + settings_.log.debug("Cut pool size: %d\n", cut_pool.pool_size()); `#endif`🤖 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 2223 - 2227, This diagnostic block is unconditionally enabled via "#if 1" and uses printf, so wrap the cut_pool.print_cutpool_types(), print_cut_types("In LP ", cut_types, settings_), and the pool size output behind the project's debug/logging guard (e.g., check a debug flag or Logger::is_debug_enabled()) instead of "#if 1"; also replace the printf call with the project's logger (e.g., processLogger.debug/info) so it respects log levels and formatting; specifically, remove the "#if 1" block, add a conditional like "if (debug_enabled)" around calls to cut_pool.print_cutpool_types(), print_cut_types(...), and the pool-size output, and use the logger API rather than printf when emitting the pool size.cpp/src/cuts/cuts.cpp (1)
725-744:⚠️ Potential issue | 🟠 MajorRemove unconditional knapsack logging from the hot path.
Line 725 (
if (1)) and Line 741 (#if 1) force verbose logging for every qualifying row, which can significantly slow separation and flood logs on large MIPs.♻️ Proposed fix
- if (1) { + if (verbose) { settings.log.printf( "Knapsack constraint %d row len %d beta %e sum_neg %e sum_pos %e sum_pos / (row_len - 1) %e\n", i, row_len, beta, sum_neg, sum_pos, sum_pos / (row_len - 1)); } @@ -#if 1 - i_t num_knapsack_constraints = knapsack_constraints_.size(); - settings.log.printf("Number of knapsack constraints %d\n", num_knapsack_constraints); -#endif + if (verbose) { + i_t num_knapsack_constraints = knapsack_constraints_.size(); + settings.log.printf("Number of knapsack constraints %d\n", num_knapsack_constraints); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 725 - 744, The code currently forces verbose logging in the knapsack detection hot path via the unconditional if (1) that calls settings.log.printf and the `#if` 1 block printing num_knapsack_constraints; remove these unconditional logs and instead wrap both the per-row printf and the final count printf in a runtime verbosity or debug flag check (e.g., settings.verbosity() > N or settings.log.isDebugEnabled()) so logging only occurs when enabled, and avoid computing expensive expressions (like sum_pos / (row_len - 1)) unless the log will be emitted; update the blocks around knapsack_constraints_, the per-row settings.log.printf call, and the num_knapsack_constraints print to use the conditional verbosity guard.
🤖 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 2223-2227: This diagnostic block is unconditionally enabled via
"#if 1" and uses printf, so wrap the cut_pool.print_cutpool_types(),
print_cut_types("In LP ", cut_types, settings_), and the pool size output
behind the project's debug/logging guard (e.g., check a debug flag or
Logger::is_debug_enabled()) instead of "#if 1"; also replace the printf call
with the project's logger (e.g., processLogger.debug/info) so it respects log
levels and formatting; specifically, remove the "#if 1" block, add a conditional
like "if (debug_enabled)" around calls to cut_pool.print_cutpool_types(),
print_cut_types(...), and the pool-size output, and use the logger API rather
than printf when emitting the pool size.
In `@cpp/src/cuts/cuts.cpp`:
- Around line 725-744: The code currently forces verbose logging in the knapsack
detection hot path via the unconditional if (1) that calls settings.log.printf
and the `#if` 1 block printing num_knapsack_constraints; remove these
unconditional logs and instead wrap both the per-row printf and the final count
printf in a runtime verbosity or debug flag check (e.g., settings.verbosity() >
N or settings.log.isDebugEnabled()) so logging only occurs when enabled, and
avoid computing expensive expressions (like sum_pos / (row_len - 1)) unless the
log will be emitted; update the blocks around knapsack_constraints_, the per-row
settings.log.printf call, and the num_knapsack_constraints print to use the
conditional verbosity guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3fc359d-9dfd-45be-9b16-50160a958299
📒 Files selected for processing (3)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hpp
…nstraints. Fix variables to reduce the size of seperation problem
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/cuts/cuts.cpp (1)
2058-2112:⚠️ Potential issue | 🟠 MajorGuard the global scaling step in
rational_coefficients().Line 696 now calls this helper on every row. When a row has no integer terms,
gcd()andlcm()both return0, soscalarbecomes NaN and the whole inequality gets scaled by NaN. On the other side, rows with many distinct denominators can overflow the accumulated scale before the laterabs(scalar) > 1000guard ever runs. Please short-circuit the no-integer case and cap the denominator accumulation before applyingscale(). As per coding guidelines "Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 2058 - 2112, The scaling step in rational_coefficients() must be guarded: if no integer terms were found (indices is empty) return true immediately to avoid gcd/lcm on empty vectors; compute gcd/ lcm only when denominators non-empty. Harden lcm() (and gcd()) to detect overflow during accumulation and clamp the result to a safe maximum (so huge denominators cannot blow up the scalar), and ensure rational_coefficients checks the computed scalar is finite, non-zero and within the allowed bounds (use std::isfinite or equivalent and the existing abs(scalar) > 1000 guard) before calling rational_inequality.scale(scalar). Update function names referenced: rational_coefficients, gcd, lcm, and the vectors numerators/denominators/indices accordingly.
🧹 Nitpick comments (1)
cpp/src/cuts/cuts.cpp (1)
726-745: Please re-hide the knapsack discovery prints.
knapsack_generation_tis constructed unconditionally fromcut_generation_t, soif (1)/#if 1make every cut setup emit per-row diagnostics and the final count even when knapsack cuts are disabled. On large models this will add avoidable overhead in a hot path and flood logs. Prefersettings.log.debug(...)or a dedicated verbose flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/cuts/cuts.cpp` around lines 726 - 745, The unconditional diagnostic prints around knapsack detection should be hidden: replace the "if (1) { settings.log.printf(...)" block with a debug-level log call (e.g. settings.log.debug(...) or whatever debug API exists) so the per-row message is emitted only when verbose/debug logging is enabled, and change the final "#if 1 ... settings.log.printf("Number of knapsack constraints %d\n", num_knapsack_constraints);" to a debug log as well (using knapsack_constraints_.size() to compute num_knapsack_constraints). Ensure you modify the printf calls around the knapsack_detection block and the final count print to use the debug logging path (or guard them with the existing verbose flag) so diagnostics are no longer emitted unconditionally.
🤖 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/cuts/cuts.cpp`:
- Around line 845-882: The loop can fix all items leaving values.empty(), but
the code still calls solve_knapsack_problem which will crash on empty input;
before calling solve_knapsack_problem check if values.empty(): if so and
seperation_rhs > 0 then set objective = 0 (the reduced knapsack optimum is zero
because any cover must come from fixed_values==1), skip calling
solve_knapsack_problem, then proceed with the existing
isNaN/restore_complemented handling; ensure you reference
solve_knapsack_problem, values, seperation_rhs, fixed_values and
restore_complemented when making the change.
In `@cpp/src/cuts/cuts.hpp`:
- Around line 169-178: The header declares two templates (rational_approximation
and rational_coefficients) but their definitions live only in cuts.cpp and are
not explicitly instantiated, causing unresolved templates for other TUs; either
move the full template definitions of rational_approximation(f_t) and
rational_coefficients<i_t,f_t> into this header (so users including cuts.hpp can
instantiate them), or remove these public declarations and keep the definitions
private in cuts.cpp and/or add explicit template instantiations in cuts.cpp for
every i_t/f_t combination you need; refer to the symbols rational_approximation,
rational_coefficients, variable_type_t and inequality_t in your change to locate
the declarations to update.
---
Outside diff comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 2058-2112: The scaling step in rational_coefficients() must be
guarded: if no integer terms were found (indices is empty) return true
immediately to avoid gcd/lcm on empty vectors; compute gcd/ lcm only when
denominators non-empty. Harden lcm() (and gcd()) to detect overflow during
accumulation and clamp the result to a safe maximum (so huge denominators cannot
blow up the scalar), and ensure rational_coefficients checks the computed scalar
is finite, non-zero and within the allowed bounds (use std::isfinite or
equivalent and the existing abs(scalar) > 1000 guard) before calling
rational_inequality.scale(scalar). Update function names referenced:
rational_coefficients, gcd, lcm, and the vectors numerators/denominators/indices
accordingly.
---
Nitpick comments:
In `@cpp/src/cuts/cuts.cpp`:
- Around line 726-745: The unconditional diagnostic prints around knapsack
detection should be hidden: replace the "if (1) { settings.log.printf(...)"
block with a debug-level log call (e.g. settings.log.debug(...) or whatever
debug API exists) so the per-row message is emitted only when verbose/debug
logging is enabled, and change the final "#if 1 ... settings.log.printf("Number
of knapsack constraints %d\n", num_knapsack_constraints);" to a debug log as
well (using knapsack_constraints_.size() to compute num_knapsack_constraints).
Ensure you modify the printf calls around the knapsack_detection block and the
final count print to use the debug logging path (or guard them with the existing
verbose flag) so diagnostics are no longer emitted unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3015e873-2776-4e76-95da-cda0669f8ff9
📒 Files selected for processing (2)
cpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hpp
| for (i_t k = 0; k < knapsack_inequality.size(); k++) { | ||
| const i_t j = knapsack_inequality.index(k); | ||
| if (!is_slack_[j]) { | ||
| const f_t vj = std::min(1.0, std::max(0.0, 1.0 - xstar[j])); | ||
| const f_t xstar_j = is_complemented[j] ? 1.0 - xstar[j] : xstar[j]; | ||
| const f_t vj = std::min(1.0, std::max(0.0, 1.0 - xstar_j)); | ||
| if (xstar_j < x_tol) { | ||
| // if xstar_j is close to 0, then we can fix z to zero | ||
| fixed_variables.push_back(j); | ||
| fixed_values.push_back(0.0); | ||
| seperation_rhs -= knapsack_inequality.vector.x[k]; | ||
| // No need to adjust the objective constant | ||
| continue; | ||
| } | ||
| if (xstar_j > 1.0 - x_tol) { | ||
| // if xstar_j is close to 1, then we can fix z to 1 | ||
| fixed_variables.push_back(j); | ||
| fixed_values.push_back(1.0); | ||
| // Note seperation rhs is unchanged | ||
| objective_constant += vj; | ||
| continue; | ||
| } | ||
| objective_constant += vj; | ||
| values[h] = vj; | ||
| weights[h] = knapsack_inequality.x[k]; | ||
| h++; | ||
| values.push_back(vj); | ||
| weights.push_back(knapsack_inequality.vector.x[k]); | ||
| indices.push_back(j); | ||
| } | ||
| } | ||
| std::vector<f_t> solution; | ||
| solution.resize(knapsack_inequality.i.size() - 1); | ||
| solution.resize(values.size()); | ||
|
|
||
| if (seperation_rhs <= 0.0) { | ||
| restore_complemented(); | ||
| return -1; | ||
| } | ||
|
|
||
| if (verbose) { settings.log.printf("Calling solve_knapsack_problem\n"); } | ||
| f_t objective = solve_knapsack_problem(values, weights, seperation_rhs, solution); | ||
| if (std::isnan(objective)) { return -1; } | ||
| if (std::isnan(objective)) { restore_complemented(); return -1; } |
There was a problem hiding this comment.
Handle the zero-variable separation case before calling the knapsack solver.
After the fixing pass, every item can be fixed while seperation_rhs is still positive on large-coefficient rows. In that case the reduced knapsack has a valid optimum of 0, and the cover can come entirely from fixed_values == 1, but solve_knapsack_problem(...) will walk an empty values array via std::max_element. This is a runtime blocker. 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".
Possible fix
std::vector<f_t> solution;
solution.resize(values.size());
if (seperation_rhs <= 0.0) {
restore_complemented();
return -1;
}
if (verbose) { settings.log.printf("Calling solve_knapsack_problem\n"); }
- f_t objective = solve_knapsack_problem(values, weights, seperation_rhs, solution);
+ f_t objective = 0.0;
+ if (!values.empty()) {
+ objective = solve_knapsack_problem(values, weights, seperation_rhs, solution);
+ } else {
+ solution.clear();
+ }
if (std::isnan(objective)) { restore_complemented(); return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/cuts/cuts.cpp` around lines 845 - 882, The loop can fix all items
leaving values.empty(), but the code still calls solve_knapsack_problem which
will crash on empty input; before calling solve_knapsack_problem check if
values.empty(): if so and seperation_rhs > 0 then set objective = 0 (the reduced
knapsack optimum is zero because any cover must come from fixed_values==1), skip
calling solve_knapsack_problem, then proceed with the existing
isNaN/restore_complemented handling; ensure you reference
solve_knapsack_problem, values, seperation_rhs, fixed_values and
restore_complemented when making the change.
| template <typename i_t, typename f_t> | ||
| bool rational_coefficients(const std::vector<variable_type_t>& var_types, | ||
| const inequality_t<i_t, f_t>& inequality, | ||
| inequality_t<i_t, f_t>& rational_inequality); | ||
|
|
||
| template <typename f_t> | ||
| bool rational_approximation(f_t x, | ||
| int64_t max_denominator, | ||
| int64_t& numerator, | ||
| int64_t& denominator); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the newly declared templates are referenced outside cpp/src/cuts/cuts.cpp
# and whether explicit instantiations exist.
echo "Usages outside cpp/src/cuts/cuts.cpp:"
rg -n -C2 '\brational_(coefficients|approximation)\s*\(' cpp -g '!cpp/src/cuts/cuts.cpp'
echo
echo "Explicit instantiations in cpp/src/cuts/cuts.cpp:"
rg -n -C2 'template\s+bool\s+rational_(coefficients|approximation)\b' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 2068
🏁 Script executed:
# First, check the size of cuts.cpp
wc -l cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 82
🏁 Script executed:
# Search for the template definitions in cuts.cpp
rg -n 'bool rational_(coefficients|approximation)\s*\(' cpp/src/cuts/cuts.cpp -A 3Repository: NVIDIA/cuopt
Length of output: 458
🏁 Script executed:
# Check if these template functions are used anywhere with explicit type instantiation
rg -n 'rational_(coefficients|approximation)\s*<' cppRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Read the template definitions to confirm they are complete implementations
sed -n '2016,2045p' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 760
🏁 Script executed:
# Read the second template definition
sed -n '2055,2095p' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 1372
🏁 Script executed:
# Check if rational_coefficients is used anywhere (it's the one that calls rational_approximation)
rg -n 'rational_coefficients\s*<' cppRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check for any usage of rational_coefficients without template syntax (might be called implicitly)
rg -n '\brational_coefficients\s*\(' cpp --type cpp --type hppRepository: NVIDIA/cuopt
Length of output: 83
🏁 Script executed:
# Check the context around the declarations in cuts.hpp to see if they're marked as internal or part of the public API
sed -n '160,185p' cpp/src/cuts/cuts.hppRepository: NVIDIA/cuopt
Length of output: 778
🏁 Script executed:
# Search for all usages of rational_coefficients more carefully
rg -n 'rational_coefficients' cpp --type-list | head -20Repository: NVIDIA/cuopt
Length of output: 445
🏁 Script executed:
# Try searching for rational_coefficients with a simpler pattern
rg -n 'rational_coefficients' cppRepository: NVIDIA/cuopt
Length of output: 658
🏁 Script executed:
# Check if knapsack_generation_t::generate_knapsack_cut uses these templates
sed -n '300,330p' cpp/src/cuts/cuts.hppRepository: NVIDIA/cuopt
Length of output: 1702
🏁 Script executed:
# Check the context of each usage to understand what types are being instantiated
sed -n '690,705p' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 557
🏁 Script executed:
# Check type of cut_A_float and cut_A used in the other calls
sed -n '1790,1800p' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 613
🏁 Script executed:
# Verify: are there any includes of cuts.hpp outside of cuts.cpp that might use these templates?
rg -l '#include.*cuts\.hpp' cpp --type c --type cppRepository: NVIDIA/cuopt
Length of output: 169
🏁 Script executed:
# Check if branch_and_bound files use rational_coefficients or rational_approximation
rg -n 'rational_(coefficients|approximation)' cpp/src/branch_and_bound/Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Verify the template definitions are truly complete and not just declarations
# Check if the definitions have closing braces and are not followed by semicolons
sed -n '2018,2050p' cpp/src/cuts/cuts.cpp | tail -20Repository: NVIDIA/cuopt
Length of output: 479
🏁 Script executed:
# Double-check: verify the actual instantiation types used in cuts.cpp
sed -n '695,700p' cpp/src/cuts/cuts.cppRepository: NVIDIA/cuopt
Length of output: 255
Move template definitions into the header or remove public declarations.
rational_approximation and rational_coefficients are declared in this public header but defined only in cpp/src/cuts/cuts.cpp with no explicit instantiations. These templates are instantiated implicitly within cuts.cpp but remain inaccessible to other translation units that might include this header. If external use is intended, move the definitions here; if these are internal helpers, keep them in cuts.cpp and remove the header declarations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/cuts/cuts.hpp` around lines 169 - 178, The header declares two
templates (rational_approximation and rational_coefficients) but their
definitions live only in cuts.cpp and are not explicitly instantiated, causing
unresolved templates for other TUs; either move the full template definitions of
rational_approximation(f_t) and rational_coefficients<i_t,f_t> into this header
(so users including cuts.hpp can instantiate them), or remove these public
declarations and keep the definitions private in cuts.cpp and/or add explicit
template instantiations in cuts.cpp for every i_t/f_t combination you need;
refer to the symbols rational_approximation, rational_coefficients,
variable_type_t and inequality_t in your change to locate the declarations to
update.
1 participant
When looking for 0-1 knapsack cuts, complement variables if the variables coefficient is negative.
This increases the number of problems where knapsack cuts were found on Wolter's test set of 20 MIPs from 5/20 to 15/20.