Skip to content

Commit

Permalink
fix: alternating brancher did not call the methods correctly (#128)
Browse files Browse the repository at this point in the history
The `AlternatingBrancher` did not call the correct methods on the
default brancher meaning that the solver had poor performance during
free search (since it did not restore any keys to the heap upon
backtracking). This has been fixed by performing the right calls
  • Loading branch information
ImkoMarijnissen authored Jan 15, 2025
1 parent e587458 commit d66ecb6
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 13 deletions.
50 changes: 39 additions & 11 deletions pumpkin-solver/src/branching/branchers/alternating_brancher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,19 @@ impl<OtherBrancher: Brancher> AlternatingBrancher<OtherBrancher> {
fn toggle_brancher(&mut self) {
self.is_using_default_brancher = !self.is_using_default_brancher
}

/// Returns true if only the default strategy is used from now on and false otherwise.
///
/// This is important if [`AlternatingStrategy::SwitchToDefaultAfterFirstSolution`] is used as
/// the strategy.
fn will_always_use_default(&self) -> bool {
match self.strategy {
AlternatingStrategy::SwitchToDefaultAfterFirstSolution => {
self.is_using_default_brancher
}
_ => false,
}
}
}

impl<OtherBrancher: Brancher> Brancher for AlternatingBrancher<OtherBrancher> {
Expand All @@ -97,13 +110,19 @@ impl<OtherBrancher: Brancher> Brancher for AlternatingBrancher<OtherBrancher> {
}

fn on_appearance_in_conflict_predicate(&mut self, predicate: Predicate) {
self.other_brancher
.on_appearance_in_conflict_predicate(predicate)
self.default_brancher
.on_appearance_in_conflict_predicate(predicate);
if !self.will_always_use_default() {
self.other_brancher
.on_appearance_in_conflict_predicate(predicate)
}
}

fn on_conflict(&mut self) {
self.other_brancher.on_conflict();
self.default_brancher.on_conflict()
self.default_brancher.on_conflict();
if !self.will_always_use_default() {
self.other_brancher.on_conflict();
}
}

fn on_solution(&mut self, solution: SolutionReference) {
Expand All @@ -120,8 +139,8 @@ impl<OtherBrancher: Brancher> Brancher for AlternatingBrancher<OtherBrancher> {
}
}
AlternatingStrategy::SwitchToDefaultAfterFirstSolution => {
// Switch only the first time, not that `even_number_of_solutions` is initialised to
// true
// Switch only the first time, note that `even_number_of_solutions` is initialised
// to true
if self.even_number_of_solutions {
self.even_number_of_solutions = false;
self.is_using_default_brancher = true;
Expand All @@ -130,12 +149,17 @@ impl<OtherBrancher: Brancher> Brancher for AlternatingBrancher<OtherBrancher> {
_ => {}
}

self.other_brancher.on_solution(solution);
self.default_brancher.on_solution(solution)
self.default_brancher.on_solution(solution);
if !self.will_always_use_default() {
self.other_brancher.on_solution(solution);
}
}

fn on_unassign_integer(&mut self, variable: DomainId, value: i32) {
self.other_brancher.on_unassign_integer(variable, value)
self.default_brancher.on_unassign_integer(variable, value);
if !self.will_always_use_default() {
self.other_brancher.on_unassign_integer(variable, value)
}
}

fn on_restart(&mut self) {
Expand Down Expand Up @@ -173,12 +197,16 @@ impl<OtherBrancher: Brancher> Brancher for AlternatingBrancher<OtherBrancher> {

fn on_backtrack(&mut self) {
self.default_brancher.on_backtrack();
self.other_brancher.on_backtrack();
if !self.will_always_use_default() {
self.other_brancher.on_backtrack();
}
}

fn synchronise(&mut self, assignments: &Assignments) {
self.default_brancher.synchronise(assignments);
self.other_brancher.synchronise(assignments);
if !self.will_always_use_default() {
self.other_brancher.synchronise(assignments);
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions pumpkin-solver/src/engine/cp/assignments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -904,7 +904,7 @@ impl IntegerDomain {
self.lower_bound_updates
.iter()
.filter(|u| u.trail_position <= trail_position)
.last()
.next_back()
.expect("Cannot fail")
.bound
}
Expand Down Expand Up @@ -945,7 +945,7 @@ impl IntegerDomain {
self.upper_bound_updates
.iter()
.filter(|u| u.trail_position <= trail_position)
.last()
.next_back()
.expect("Cannot fail")
.bound
}
Expand Down

0 comments on commit d66ecb6

Please sign in to comment.