Block Bounded Value Elimination presolving pass for binary problems - #1683
Open
aliceb-nv wants to merge 34 commits into
Open
Block Bounded Value Elimination presolving pass for binary problems#1683aliceb-nv wants to merge 34 commits into
aliceb-nv wants to merge 34 commits into
Conversation
aliceb-nv
added
non-breaking
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesBlock-BVE presolve
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 10
🧹 Nitpick comments (3)
cpp/src/mip_heuristics/presolve/block_bve.cu (1)
1321-1321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider preserving the row names of the surviving rows.
The call passes
{}for the names, soset_constraints_from_host_csrclearsrow_namesfor the whole model. The rows that block-BVE keeps are unchanged, and their names are still available inproblem.row_namesbefore the call. After the call,mps_writer_t::writeand the log messages fall back to generated names such asR12, which makes an exported reduced model hard to compare with the input.Filter the existing names for the kept rows, and generate names for the appended clause rows.
♻️ Proposed name mapping
+ std::vector<std::string> new_names; + if (problem.row_names.size() == (size_t)n_rows) { + new_names.reserve(n_rows + plan.added_rows.size()); + for (i_t r = 0; r < n_rows; ++r) + if (!removed[r]) new_names.push_back(problem.row_names[r]); + for (size_t c = 0; c < plan.added_rows.size(); ++c) + new_names.push_back("bve_nogood_" + std::to_string(c)); + } work_units += double(new_var.size()) + double(new_clb.size()); - problem.set_constraints_from_host_csr(new_off, new_var, new_coef, new_clb, new_cub, {}); + problem.set_constraints_from_host_csr(new_off, new_var, new_coef, new_clb, new_cub, new_names);🤖 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/mip_heuristics/presolve/block_bve.cu` at line 1321, Preserve row names when rebuilding constraints in the block-BVE flow: derive names for the surviving original rows from problem.row_names using the same kept-row mapping as new_off/new_var/new_coef, append generated names for newly added clause rows, and pass the resulting names instead of {} to set_constraints_from_host_csr. Ensure the name list matches the rebuilt constraint count and preserves existing names for unchanged rows.cpp/src/mip_heuristics/presolve/probing_cache.cu (1)
194-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the cache-presence test.
Line 196 reads
if (!bound_presolve.probing_cache.probing_cache.count(var_original) > 0). This parses as(!count(...)) > 0, which is true only when the count is zero. The result matches the intent, but the expression is misleading for a reader. Use an explicit form.♻️ Proposed clarification
- if (!bound_presolve.probing_cache.probing_cache.count(var_original) > 0) { + if (bound_presolve.probing_cache.probing_cache.count(var_original) == 0) {🤖 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/mip_heuristics/presolve/probing_cache.cu` around lines 194 - 201, Clarify the cache-presence condition in the probing-cache insertion logic by replacing the negated count comparison with an explicit zero-count check. Update the condition surrounding probing_cache.count(var_original); keep the existing insertion and update branches unchanged.cpp/src/mip_heuristics/problem/presolve_data.cu (1)
143-160: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd asserts for the interior width and the witness validity.
Two invariants of the BlockBve branch are unchecked:
wis auint32_t, sorec.bve.interior.size()must not exceed 32. IfBVE_MAX_INTERIORis ever raised above 32, the loop silently reconstructs zeros for the extra positions. The existing assert covers only the boundary width.rec.bve.witness[pattern]is only written for boundary patterns the projection found feasible. For an infeasible pattern the slot keeps itsatomicMininitializer, and the reconstruction writes all ones. The added no-good clauses make that unreachable for a feasible reduced assignment, so an assert is the right way to record the invariant.🛡️ Proposed asserts
case reconstruction_kind_t::BlockBve: { cuopt_assert(rec.bve.witness.size() == (size_t{1} << rec.bve.boundary.size()), "block witness size mismatch"); + cuopt_assert(rec.bve.interior.size() <= 32, + "block interior wider than the witness bit width"); uint32_t pattern = 0; for (size_t j = 0; j < rec.bve.boundary.size(); ++j) { cuopt_assert(rec.bve.boundary[j] < (i_t)h_assignment.size(), "block boundary out of bounds"); const int bit = (h_assignment[rec.bve.boundary[j]] > static_cast<f_t>(0.5)) ? 1 : 0; pattern |= (static_cast<uint32_t>(bit) << j); } const uint32_t w = rec.bve.witness[pattern]; + cuopt_assert(w != std::numeric_limits<uint32_t>::max(), + "boundary pattern has no feasible interior witness");🤖 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/mip_heuristics/problem/presolve_data.cu` around lines 143 - 160, In the BlockBve reconstruction branch, add an assertion that rec.bve.interior.size() does not exceed 32 before shifting bits from the uint32_t witness. After selecting rec.bve.witness[pattern], assert that the witness is not the atomicMin sentinel representing an infeasible boundary pattern, then preserve the existing interior reconstruction.
🤖 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/math_optimization/solver_settings.cu`:
- Around line 214-216: Update the block-BVE loop guard to require
run_probing_cache, adding !run_probing_cache to the existing condition so
block_bve_presolve is skipped when probing is disabled. Preserve the current BVE
behavior when the probing cache is enabled.
In `@cpp/src/mip_heuristics/diversity/diversity_manager.cu`:
- Around line 364-373: Guard the update_variable_bounds call in the surrounding
modification function so it runs only when var_indices is non-empty, matching
apply_modification_queue_to_problem. Keep n_applied assigned from
var_indices.size() and preserve the existing return behavior.
- Around line 481-494: Update the GPU-export block in solve_mip around
problem_to_mps_data_model and mps_writer_t::write to avoid calling exit from
library code; return through the normal solver path and propagate export success
or failure to the caller. Check the result of writer.write and report failures
instead of treating them as successful completion. Sanitize instance_name before
forming mps_path so model-provided path separators cannot escape the working
directory.
In `@cpp/src/mip_heuristics/presolve/block_bve.cu`:
- Around line 1111-1135: Make the deterministic-order fixes in
cpp/src/mip_heuristics/presolve/block_bve.cu at lines 1111-1135 and 716-717:
sort each adjacency list in the helper that builds and returns out, and have
grow_seed_interior iterate a sorted copy of cands_w for stable boundary-size
tie-breaking. In the per-bin work estimate at lines 716-717, accumulate
operation counts as an integer and convert to double only once after summing, so
the result is independent of bins iteration order.
- Around line 1015-1027: Update the growth loop around grow_seed_interior to use
taskloop when block_bve_presolve is called from an existing OpenMP team,
avoiding nested teams, while retaining a parallel-for fallback for callers
outside any team. Preserve the current per-seed growth_done handling, result
assignment, and interior moves in both execution paths.
In `@cpp/src/mip_heuristics/problem/problem.cu`:
- Around line 2113-2128: Update the empty-state assignment near nnz
initialization to match op_problem_cstr_body: mark the problem empty whenever
the constraint matrix has no entries, including n_constraints == 0 with
remaining variables. Preserve the existing nnz calculation and validation
checks, and anchor the change to the empty assignment in the constructor
handling variables_in and coefficients_in.
- Line 208: Update the initialization of objective_offset in the affected
problem reconstruction flow so that when no_deep_copy is false it uses the same
reconstructed presolve_data source as the corresponding presolve_data field,
rather than problem_.presolve_data.objective_offset. Keep objective_offset
consistent with the rebuilt data after presolve.
In `@cpp/tests/mip/block_bve_test.cu`:
- Around line 797-801: In the test block that reconstructs recon_obj, assert
that m_obj.size() equals full.size() before iterating. Then iterate over the
full objective vector without the mismatched-size guard, preserving the existing
EXPECT_NEAR comparison.
- Around line 666-678: Make the size guard in brute_force_binary fatal by
replacing EXPECT_LE(nv, 24) with ASSERT_LE so oversized reduced models return
before the exponential loop. Because ASSERT_* requires a void-returning
function, update brute_force_binary and its callers to use an out-parameter for
bve_bf_t while preserving the existing result behavior.
In `@skills/cuopt-developer/SKILL.md`:
- Line 174: Re-run NVSkills validation for the updated skill set and refresh the
signature commit so it covers the current SKILL.md and related skill files.
Update the workflow around the skill package validation/signing step to
regenerate skill.oms.sig after the latest push, and keep the resulting signature
commit in the branch history as the expected artifact.
---
Nitpick comments:
In `@cpp/src/mip_heuristics/presolve/block_bve.cu`:
- Line 1321: Preserve row names when rebuilding constraints in the block-BVE
flow: derive names for the surviving original rows from problem.row_names using
the same kept-row mapping as new_off/new_var/new_coef, append generated names
for newly added clause rows, and pass the resulting names instead of {} to
set_constraints_from_host_csr. Ensure the name list matches the rebuilt
constraint count and preserves existing names for unchanged rows.
In `@cpp/src/mip_heuristics/presolve/probing_cache.cu`:
- Around line 194-201: Clarify the cache-presence condition in the probing-cache
insertion logic by replacing the negated count comparison with an explicit
zero-count check. Update the condition surrounding
probing_cache.count(var_original); keep the existing insertion and update
branches unchanged.
In `@cpp/src/mip_heuristics/problem/presolve_data.cu`:
- Around line 143-160: In the BlockBve reconstruction branch, add an assertion
that rec.bve.interior.size() does not exceed 32 before shifting bits from the
uint32_t witness. After selecting rec.bve.witness[pattern], assert that the
witness is not the atomicMin sentinel representing an infeasible boundary
pattern, then preserve the existing interior reconstruction.
🪄 Autofix
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: 6f1d63c3-3198-40af-ad58-d8487e872367
⛔ Files ignored due to path filters (12)
datasets/mip/block_bve/and_used.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/aux_with_obj.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/chain_or.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/heavy_reduce.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/infeasible.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/mixed.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/neq_used.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/or_used.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/random_a.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/random_b.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/random_c.mpsis excluded by!**/*.mpsdatasets/mip/block_bve/two_gadgets.mpsis excluded by!**/*.mps
📒 Files selected for processing (21)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/include/cuopt/mathematical_optimization/mip/solver_settings.hppcpp/src/io/mps_writer.cppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/CMakeLists.txtcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/diversity/population.cucpp/src/mip_heuristics/presolve/block_bve.cucpp/src/mip_heuristics/presolve/block_bve.cuhcpp/src/mip_heuristics/presolve/probing_cache.cucpp/src/mip_heuristics/presolve/probing_cache.cuhcpp/src/mip_heuristics/problem/presolve_data.cucpp/src/mip_heuristics/problem/presolve_data.cuhcpp/src/mip_heuristics/problem/problem.cucpp/src/mip_heuristics/problem/problem.cuhcpp/src/mip_heuristics/solve.cucpp/src/utilities/integer_scaling.hppcpp/tests/internal/CMakeLists.txtcpp/tests/mip/block_bve_test.cuskills/cuopt-developer/SKILL.mdskills/cuopt-developer/references/conventions.md
Contributor
Author
|
/ok to test 0abe89c |
CI Test Summary✅ All 31 test job(s) passed. |
Contributor
Author
|
/ok to test 4dc1e57 |
Contributor
Author
|
/ok to test acac6b5 |
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.
This PR adds a binary reduction presolving pass inspired by SAT-related bounded variable elimination work such as "Effective Preprocessing in SAT through Variable and Clause Elimination" (SAT 2005).
Block BVE eliminates a small set of non-objective binary variables (the interior) by projecting the constraints they appear in onto the other binary variables of those same rows (the boundary), which stay in the model.
For every assignment of the boundary it checks if the interior can be set to satisfy each row of the block.
The assignments admitting no such setting are everything the block still forces on the rest of the
model, so these can be turned into no-good rows while eliminating the interior columns altogether. Reductions requiring new no-good clauses are only committed if they would result in fewer total rows, to avoid excessive growth.
Candidates come from the probing cache. Each non-objective binary carrying at least one edge becomes a seed, tried in order of increasing row count, grown one variable at a time: among the eligible implication neighbors of the current interior, it absorbs whichever leaves the smallest boundary, and only while the boundary strictly shrinks.
Each grown interior is staged into a block and dropped if it breaks preset caps. Disjoint candidates are batched and enumerated on the GPU, yielding a feasibility bit and witness interior per boundary pattern, and the derived clauses are checked against that table and checked for growth.
Results are as follows on MIPLIB2017. Most reductions occur on mostly-binary combinatorial problems like the bnatt class, the piperout class, and the cryptanalysis classes.
We now find +1 feasible/optimal: bnatt400 The BKS is found in ~5s, and optimality proven after B&B in ~2min.
Description
Issue
Checklist