MIP symmetry detection and orbital fixing using dejavu#1103
Open
chris-maes wants to merge 10 commits intoNVIDIA:mainfrom
Open
MIP symmetry detection and orbital fixing using dejavu#1103chris-maes wants to merge 10 commits intoNVIDIA:mainfrom
chris-maes wants to merge 10 commits intoNVIDIA:mainfrom
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
Contributor
|
Wow, those are some good numbers indeed 🙂 Thanks a lot Chris!! |
Contributor
|
Maybe @aliceb-nv would be a better reviewer for this than me? (Looks like I got pulled in by auto-assignment) |
Contributor
Author
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
Comment on lines
+1816
to
1818
| 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
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/pseudo_costs.cpp`:
- Around line 1290-1317: strong_branching_neccesary() only fills
pc.strong_branch_down/up for necessary_fractional but the code later calls
pc.update_pseudo_costs_from_strong_branching(fractional, ...) and copies into
pc.pseudo_cost_* using the wrong buffers, causing out-of-bounds reads and zeros;
fix by first expanding/propagating the delegate-backed strong_branch_down/up
values into the full fractional ordering before copying into pseudo_cost_sum/up
and pseudo_cost_num/up: after strong_branching_neccesary() iterate over
fractional, find delegate = orbit_delegate[symmetry->orbit_rep[j]], if
j==delegate set pc.strong_branch_down[j]=pc.strong_branch_down[delegate] and
pc.strong_branch_up[j]=pc.strong_branch_up[delegate] (or resize
pc.strong_branch_* to fractional.size() and populate via mapping), then copy
those propagated pc.strong_branch_down/up values into pc.pseudo_cost_sum_down/up
and set pc.pseudo_cost_num_down/up accordingly, and only then call
pc.update_pseudo_costs_from_strong_branching(fractional, root_solution.x).
In `@cpp/src/branch_and_bound/symmetry.hpp`:
- Around line 584-677: The has_symmetry flag is being set from the full-graph
generator count before you project away generators that move continuous vars, so
symmetry may be reported even when the projected integer-only Schreier is
trivial; update the logic to recompute or reset has_symmetry after the
projection/filter step (the code that discards generators moving continuous
variables / after you inspect result->num_generators or the projected schreier),
and if the projected generator count is zero set has_symmetry = false (or return
nullptr / set mip_symmetry_t to null) so downstream code won’t take
symmetry-specific paths for an unusable symmetry; adjust any related
orbit/statistics reporting (schreier, orb, num_generators,
result->num_generators, mip_symmetry_t) to use the post-projection data.
- Around line 213-223: The code currently can push the same variable index into
both fix_zero and fix_one because the orbit predicates overlap; change the logic
to compute two booleans (e.g., can_fix_zero = (orbit_has_b0_[o] ||
orbit_has_f0_[o]) && marked_b0_[j]==0 && marked_f0_[j]==0 and can_fix_one =
orbit_has_f1_[o] && marked_f1_[j]==0), then if both can_fix_zero and can_fix_one
are true treat this as a contradiction (do not push the index into either vector
and set/return an appropriate contradiction flag) otherwise push to fix_zero or
fix_one exclusively; update any downstream code that assumes elements in
fix_zero/fix_one are disjoint (the block that applies bounds for these lists) to
rely on this exclusive behavior.
🪄 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: 2b37dc72-edc1-4d1e-bc22-ec78579c38dd
📒 Files selected for processing (5)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound_worker.hppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/branch_and_bound/pseudo_costs.hppcpp/src/branch_and_bound/symmetry.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/branch_and_bound/branch_and_bound_worker.hpp
- cpp/src/branch_and_bound/branch_and_bound.cpp
Comment on lines
+213
to
+223
| if (orbit_has_b0_[o] == 1 || orbit_has_f0_[o] == 1) { | ||
| // The orbit of this variable contains variables in B0 or F0 | ||
| // So we can fix this variable to zero (provided its not already in B0 or F0) | ||
| if (marked_b0_[j] == 0 && marked_f0_[j] == 0) { fix_zero.push_back(j); } | ||
| } | ||
|
|
||
| if (orbit_has_f1_[o] == 1) { | ||
| // The orbit of this variable contains variables in F1 | ||
| // So we can fix this variable to one (provided its not already in F1) | ||
| if (marked_f1_[j] == 0) { fix_one.push_back(j); } | ||
| } |
There was a problem hiding this comment.
Avoid pushing the same variable into both fix_zero and fix_one.
These predicates are not mutually exclusive. If an orbit already contains both an F0/B0 witness and an F1 witness, an unfixed member of that orbit is added to both lists, and Lines 259-266 overwrite its bounds inconsistently. At minimum, make the two cases exclusive and treat can_fix_zero && can_fix_one as a contradiction instead of applying both fixings.
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."
Also applies to: 259-266
🤖 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 213 - 223, The code
currently can push the same variable index into both fix_zero and fix_one
because the orbit predicates overlap; change the logic to compute two booleans
(e.g., can_fix_zero = (orbit_has_b0_[o] || orbit_has_f0_[o]) && marked_b0_[j]==0
&& marked_f0_[j]==0 and can_fix_one = orbit_has_f1_[o] && marked_f1_[j]==0),
then if both can_fix_zero and can_fix_one are true treat this as a contradiction
(do not push the index into either vector and set/return an appropriate
contradiction flag) otherwise push to fix_zero or fix_one exclusively; update
any downstream code that assumes elements in fix_zero/fix_one are disjoint (the
block that applies bounds for these lists) to rely on this exclusive behavior.
… the whole stab(G, B1) but rather a subgroup. Speeds up time for each node. But misses fixings.
… full stabilizer(G, B1)
…d strengthening. Fixes an issue on lectsched-5-obj
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
4 participants
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 add supports for symmetry detection in MIP.
We construct a colored graph for MIP problems that can be used with a graph automorphism solver (in this case dejavu) to detect symmetries.
Below we compare with the paper: A Computational Comparison of Symmetry Handling Methods for Mixed Integer Programs
https://optimization-online.org/wp-content/uploads/2015/11/5209.pdf