Detect MIP symmetry using dejavu; Perform orbital fixing and lexical reduction#1103
Conversation
chris-maes
added
feature request
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds graph-based MIP symmetry detection using dejavu, a new symmetry header/types with orbit detection and orbital-fixing, wires detection into MIP solve (pre-presolve), threads symmetry into branch-and-bound and strong-branching (orbit-aware), and updates CMake/tests to fetch/include dejavu. 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)
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: 3
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/symmetry.hpp (1)
62-66: Inconsistent floating-point comparisons for coloring.The code uses
!=for exact comparison of bounds (lines 64-65, 78-79) but uses tolerance-based comparison for objectives (lines 62, 78). This inconsistency could lead to different variables being assigned different colors when they should be equivalent (or vice versa).Consider using tolerance-based comparison consistently:
♻️ Suggested approach
- if (problem.lower[a] != problem.lower[b]) return problem.lower[a] < problem.lower[b]; - if (problem.upper[a] != problem.upper[b]) return problem.upper[a] < problem.upper[b]; + if (std::abs(problem.lower[a] - problem.lower[b]) > tol) return problem.lower[a] < problem.lower[b]; + if (std::abs(problem.upper[a] - problem.upper[b]) > tol) return problem.upper[a] < problem.upper[b];And similarly for the color assignment comparisons on lines 78-79.
Also applies to: 78-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/symmetry.hpp` around lines 62 - 66, The comparisons for bounds in symmetry.hpp are using exact != and < while objectives use tolerance-based comparison, causing inconsistent coloring; update the comparator and the color-assignment code to use a consistent floating-point tolerance (e.g., a local constexpr double tol or an existing epsilon) when comparing problem.objective, problem.lower, and problem.upper so that equality checks use fabs(x-y) <= tol and ordering uses x < y - tol (or x > y + tol) consistently; apply the same tolerant comparisons in the color-assignment branch that currently compares problem.lower and problem.upper and keep integer/enum comparisons (var_types) unchanged to ensure stable, consistent coloring across objective and bound checks.
🤖 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/branch_and_bound/symmetry.hpp`:
- Around line 145-146: Guard accesses to nonzero_perm[0] when there are no
nonzeros: before using nonzero_perm[0] (where last_nz and nonzero_colors are
set) check that nnz > 0 and skip the assignment/usage if nnz == 0; apply the
same guard in both the FULL_GRAPH branch (where last_nz =
nonzeros[nonzero_perm[0]] and nonzero_colors[nonzero_perm[0]] = edge_color) and
the default branch (the other occurrence of nonzero_perm[0]). Locate the code in
symmetry.hpp around the nonzero_perm/nonzeros initialization and add a simple if
(nnz > 0) { ... } around these statements so they cannot index into an empty
nonzero_perm array.
- Around line 70-74: The code accesses obj_perm[0] and related arrays without
guarding for an empty problem, which can cause OOB when problem.num_cols == 0;
add an early guard at the start of the routine containing these lines (check
problem.num_cols == 0 or obj_perm.empty()) and return/skip the symmetry setup if
true so no reads of obj_perm[0], problem.objective, problem.lower/upper,
var_types or writes to var_colors occur; ensure the guard is placed before the
block that assigns last_obj, last_lower, last_upper, last_type and
var_colors[obj_perm[0]] = var_color.
In `@cpp/src/mip_heuristics/solve.cu`:
- Around line 310-319: The symmetry detection block (creating detail::problem_t,
simplex_settings, and calling dual_simplex::detect_symmetry) runs before timer_t
is created so its runtime isn't counted toward settings.time_limit and it runs
even when presolve is disabled; move this entire block to after the timer_t
initialization so detect_symmetry uses the started timer and the remaining
time_limit, and additionally wrap the block with a guard that checks
settings.presolver != presolver_t::None (or otherwise only run when presolve is
enabled) so symmetry detection is conditional on the presolver setting.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/symmetry.hpp`:
- Around line 62-66: The comparisons for bounds in symmetry.hpp are using exact
!= and < while objectives use tolerance-based comparison, causing inconsistent
coloring; update the comparator and the color-assignment code to use a
consistent floating-point tolerance (e.g., a local constexpr double tol or an
existing epsilon) when comparing problem.objective, problem.lower, and
problem.upper so that equality checks use fabs(x-y) <= tol and ordering uses x <
y - tol (or x > y + tol) consistently; apply the same tolerant comparisons in
the color-assignment branch that currently compares problem.lower and
problem.upper and keep integer/enum comparisons (var_types) unchanged to ensure
stable, consistent coloring across objective and bound checks.
🪄 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: Pro Plus
Run ID: a427358a-faac-4982-8eb3-86dd4a92ed37
📒 Files selected for processing (3)
cpp/CMakeLists.txtcpp/src/branch_and_bound/symmetry.hppcpp/src/mip_heuristics/solve.cu
|
Wow, those are some good numbers indeed 🙂 Thanks a lot Chris!! |
|
Maybe @aliceb-nv would be a better reviewer for this than me? (Looks like I got pulled in by auto-assignment) |
Good question @aliceb-nv , I think it is more difficult to construct the graph to compute the automorphism using the representation in problem_t. In particular the lower and upper bounds on the constraints complicate things. My strategy was to extract the symmetries in the variables from the symmetries computed on the graph. I think once we have symmetries on the variables, we can use them inside presolve. Note that we may need to turn off some (perhaps all) of presolve if we detect symmetries. |
…a bug in branch and bound.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Around line 1396-1547: Deterministic B&B never runs the orbital-fixing block
(it only runs in solve_node_lp), so either move the orbital-fixing logic into a
reused helper and call it from solve_node_deterministic() and
deterministic_dive(), or disable symmetry_ in deterministic mode; specifically,
extract the symmetry handling/fixing code currently inside the if (symmetry_ !=
nullptr) block into a new function (e.g., perform_orbital_fixing(node_ptr,
worker) or symmetry_->apply_orbital_fixing(...)) and invoke that helper from
solve_node_deterministic() and deterministic_dive() (or set symmetry_ = nullptr
/ skip symmetry setup when deterministic) so deterministic runs no longer pay
overhead without applying fixes. Ensure the helper uses the same symbols
(node_ptr, worker, symmetry_, var_types_, settings_.log) and preserves all
marking/restore semantics.
- Around line 1396-1547: The code mutates shared symmetry_ state
(symmetry_->marked_b0/marked_b1/marked_f0/marked_f1, symmetry_->orbit_has_*,
symmetry_->f0/f1 and calls symmetry_->schreier->set_base) while solve_node_lp()
runs concurrently, causing races; fix by making all scratch state worker-local
and avoid mutating the shared Schreier base: allocate per-worker vectors (e.g.
local_marked_b0, local_marked_b1, local_marked_f0, local_marked_f1,
local_orbit_has_*, local_f0, local_f1) sized symmetry_->num_original_vars and
use those instead of symmetry_->marked_* and symmetry_->orbit_has_*, and either
use a worker-local copy/clone of symmetry_->schreier (or ask Schreier for a
temporary stabilizer/orbit API) and call set_base on that copy; ensure you
update worker-local data when computing orb and apply fixings to
worker->leaf_problem as before; if cloning schreier is impossible, protect the
set_base/get_stabilizer_orbit sequence with a mutex around symmetry_->schreier
to prevent concurrent mutation.
In `@cpp/src/branch_and_bound/symmetry.hpp`:
- Around line 405-430: The code uses orb.orbit_size(j) (which counts all
vertices including slacks/aux) to build orbit_histogram and compute
max_orbit_size/total_vars_in_orbits; this can exceed num_original_vars and
corrupt orbit_histogram. Fix by counting only original variables per orbit
representative: when orb.represents_orbit(j) is true, accumulate a count per
representative (e.g., a map keyed by j) by incrementing for each original index
j, then after collecting counts use that representative-count as sz_original
(bounded by num_original_vars) to update orbit_histogram, max_orbit_size, and
total_vars_in_orbits, and base the has_symmetry test on those original-variable
counts instead of orb.orbit_size(j).
🪄 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: Pro Plus
Run ID: 994d1ff5-8392-4cf9-9414-93016c15e887
📒 Files selected for processing (8)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/symmetry.hppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver.cucpp/src/mip_heuristics/solver_context.cuhcpp/tests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (2)
- cpp/src/mip_heuristics/diversity/diversity_manager.cu
- cpp/tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/mip_heuristics/solve.cu
…ngle thread. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Around line 1530-1545: The current orbital-fixing logging
(settings_.log.printf) is too verbose in the hot solve path: avoid printing the
per-variable lines for each v in fix_zero/fix_one and make the summary
conditional or lower-severity; update the block around node_ptr->node_id /
fix_zero / fix_one so only the summary is logged at a debug/trace level (or
logged only when fix count > 0 or a verbose flag is set) and remove or guard the
per-variable logs that print each fix and the repeated accesses to
worker->leaf_problem.lower/upper to prevent log flooding and performance
degradation.
🪄 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: Pro Plus
Run ID: 2f6896ee-3fe4-4af8-8f94-31fcd953119b
📒 Files selected for processing (1)
cpp/src/branch_and_bound/branch_and_bound.cpp
…object. Make orbital fixing work with multiple threads. Allow for general integers (but only fix binary).
chris-maes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
1396-1400:⚠️ Potential issue | 🟠 MajorDeterministic B&B still bypasses orbital fixing.
Orbital fixing is only executed in
solve_node_lp()(nondeterministic/scheduler flow). Deterministic flows (solve_node_deterministic()/deterministic_dive()) still do not apply it, so symmetry detection can add overhead there without downstream fixing benefit.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."
🤖 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 1396 - 1400, Deterministic B&B paths skip orbital fixing: add the same orbital fixing invocation used in solve_node_lp to the deterministic flows by checking worker->orbital_fixing (via auto* of = worker->orbital_fixing.get()) and calling of->orbital_fixing(symmetry_, settings_, var_types_, node_ptr, worker->leaf_problem) inside solve_node_deterministic() and deterministic_dive() where node processing occurs (mirror the conditional used in branch_and_bound.cpp around the current orbital fixing call) so symmetry detection overhead yields actual fixing in deterministic branches.
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/symmetry.hpp (1)
116-135: Use tolerance checks for 0/1 bound tests in orbital-fixing predicates.These exact comparisons can miss valid fixing inputs when propagated bounds are numerically near 0/1.
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."♻️ Proposed refinement
+ const f_t eps = settings.integer_tol; while (node != nullptr && node->branch_var >= 0) { i_t v = node->branch_var; bool is_binary = (symmetry->is_binary[v] == 1); if (is_binary) { - if (node->branch_var_upper == 0.0) { + if (std::abs(node->branch_var_upper) <= eps) { branched_zero.push_back(v); marked_b0_[v] = 1; - } else if (node->branch_var_lower == 1.0) { + } else if (std::abs(node->branch_var_lower - f_t(1.0)) <= eps) { branched_one.push_back(v); marked_b1_[v] = 1; } } node = node->parent; } @@ - if (marked_b1_[j] == 0 && problem.lower[j] == 1.0) { + if (marked_b1_[j] == 0 && std::abs(problem.lower[j] - f_t(1.0)) <= eps) { f1_.push_back(j); marked_f1_[j] = 1; } - if (marked_b0_[j] == 0 && problem.upper[j] == 0.0) { + if (marked_b0_[j] == 0 && std::abs(problem.upper[j]) <= eps) { f0_.push_back(j); marked_f0_[j] = 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/symmetry.hpp` around lines 116 - 135, The comparisons against exact 0.0/1.0 in the orbital-fixing logic can miss near-bound values due to floating-point error; update the tests in the loop that fills f1_/f0_ (currently checking problem.lower[j] == 1.0 and problem.upper[j] == 0.0) to use an epsilon tolerance (e.g., problem.lower[j] >= 1.0 - eps and problem.upper[j] <= eps) or the project's existing numeric tolerance constant, and keep the marking updates (marked_f1_, marked_f0_, pushes to f1_/f0_) unchanged when the tolerant check passes so near-1/0 bounds are correctly detected.
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Around line 1816-1818: single_threaded_solve() takes an idle worker via
worker_pool_.get_idle_worker() and calls plunge_with(worker) but never resets
that worker's active state, which lets a stale worker->lower_bound leak into
worker_pool_.get_lower_bound(); after any call that uses plunge_with(worker)
(and the other similar occurrence) explicitly return the worker to the pool or
mark it inactive (e.g., call the pool release method or
worker->set_inactive()/worker_pool_.release_worker(worker)) so the worker_pool_
no longer aggregates its stale lower_bound; update both the initial usage
(around worker_pool_.init(...) / get_idle_worker() / plunge_with(worker)) and
the later occurrence to ensure workers are reset after plunges.
---
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1396-1400: Deterministic B&B paths skip orbital fixing: add the
same orbital fixing invocation used in solve_node_lp to the deterministic flows
by checking worker->orbital_fixing (via auto* of = worker->orbital_fixing.get())
and calling of->orbital_fixing(symmetry_, settings_, var_types_, node_ptr,
worker->leaf_problem) inside solve_node_deterministic() and deterministic_dive()
where node processing occurs (mirror the conditional used in
branch_and_bound.cpp around the current orbital fixing call) so symmetry
detection overhead yields actual fixing in deterministic branches.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/symmetry.hpp`:
- Around line 116-135: The comparisons against exact 0.0/1.0 in the
orbital-fixing logic can miss near-bound values due to floating-point error;
update the tests in the loop that fills f1_/f0_ (currently checking
problem.lower[j] == 1.0 and problem.upper[j] == 0.0) to use an epsilon tolerance
(e.g., problem.lower[j] >= 1.0 - eps and problem.upper[j] <= eps) or the
project's existing numeric tolerance constant, and keep the marking updates
(marked_f1_, marked_f0_, pushes to f1_/f0_) unchanged when the tolerant check
passes so near-1/0 bounds are correctly detected.
🪄 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: Pro Plus
Run ID: 364e4ef3-4ef1-434b-8a2b-80dd066dc4ff
📒 Files selected for processing (7)
cpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound_worker.hppcpp/src/branch_and_bound/symmetry.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/solve.cu
✅ Files skipped from review due to trivial changes (2)
- cpp/include/cuopt/linear_programming/constants.h
- cpp/src/mip_heuristics/solve.cu
| worker_pool_.init(1, original_lp_, Arow_, var_types_, symmetry_, settings_); | ||
| branch_and_bound_worker_t<i_t, f_t>* worker = worker_pool_.get_idle_worker(); | ||
|
|
There was a problem hiding this comment.
Single-thread worker can remain active with stale lower bound after plunge.
single_threaded_solve() now uses worker_pool_, but this path never resets the worker to inactive after plunge_with(worker). Since get_lower_bound() aggregates worker_pool_.get_lower_bound(), stale worker->lower_bound can leak into gap/lower-bound reporting across iterations.
🔧 Proposed fix
worker->init_best_first(start_node.value(), original_lp_);
plunge_with(worker);
+ worker->is_active = false; // prevent stale worker bound from affecting global lower boundAlso applies to: 1860-1861
🤖 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 1816 - 1818,
single_threaded_solve() takes an idle worker via worker_pool_.get_idle_worker()
and calls plunge_with(worker) but never resets that worker's active state, which
lets a stale worker->lower_bound leak into worker_pool_.get_lower_bound(); after
any call that uses plunge_with(worker) (and the other similar occurrence)
explicitly return the worker to the pool or mark it inactive (e.g., call the
pool release method or
worker->set_inactive()/worker_pool_.release_worker(worker)) so the worker_pool_
no longer aggregates its stale lower_bound; update both the initial usage
(around worker_pool_.init(...) / get_idle_worker() / plunge_with(worker)) and
the later occurrence to ensure workers are reset after plunges.
…ract the subgroup on the variables from the symmetry group on the graph
…s accidently being enabled in diving
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Approving CMAKE changes
…se fixings in lexical reduction. Track pruned nodes in lexical reduction
|
🔔 Hi @anandhkb @chris-maes, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
aliceb-nv
left a comment
aliceb-nv
left a comment
There was a problem hiding this comment.
Amazing work Chris. A few comments, but otherwise LGTM.
Maybe we could consider adding a few unit tests. It seems to me that it would be all too easy to accidentally break the symmetry handling code or its assumptions. Maybe we could test on some files from the Margot datasets, or create some synthetic ones.
|
🔔 Hi @anandhkb @chris-maes, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
…ry_detection Resolve multiple merge conflicts. Also fix bug in psuedo costs translation from reduced to full set of fractional
|
/ok to test a7126e4 |
chris-maes
changed the title
chris-maes
changed the title
|
/ok to test c890f2f |
|
/merge |
rapids-bot
Bot
merged commit f173aa2
into
NVIDIA:release/26.06
6 participants
This PR add supports for symmetry detection in MIP. If symmetry is detected we apply orbital fixing and lexical reduction inside branch and bound.
We construct a colored graph for MIP problems that can be used with a graph automorphism solver (in this case dejavu) to detect symmetries.
We perform symmetry detection after presolve. Note that presolve may have already destroyed symmetries. But presolve is crucial for good performance.
We then implement orbital fixing and lexical reduction within the branch and bound tree.
There is a long symmetry funnel.
240 MIPLIB instances
-> 59 instances had exploitable symmetry
-> 42 instances had symmetry and were not solved at the root node
-> 33 instances had orbital fixing (12 instances had lexical reduction)
-> 7 instances solved to optimality in 3600 seconds
Overall the change is performance neutral. But we are now able to solve two more problems to optimality:
cod105andmzzv42z