From c33ff51b53e58feeab559c39fa49f0f540f18e07 Mon Sep 17 00:00:00 2001 From: Remo Christen Date: Wed, 10 Jul 2024 00:31:20 +0200 Subject: [PATCH 1/8] [issue1140] Adapt cheaper path reinsertion condtion and rename node operations. --- src/search/search_algorithms/eager_search.cc | 48 +++++++++++-------- .../enforced_hill_climbing_search.cc | 3 +- src/search/search_algorithms/lazy_search.cc | 7 ++- src/search/search_space.cc | 42 +++++++--------- src/search/search_space.h | 18 ++++--- 5 files changed, 63 insertions(+), 55 deletions(-) diff --git a/src/search/search_algorithms/eager_search.cc b/src/search/search_algorithms/eager_search.cc index 5ec760e577..be135323b9 100644 --- a/src/search/search_algorithms/eager_search.cc +++ b/src/search/search_algorithms/eager_search.cc @@ -139,22 +139,24 @@ SearchStatus EagerSearch::step() { With lazy evaluators (and only with these) we can have dead nodes in the open list. - For example, consider a state s that is reached twice before it is expanded. - The first time we insert it into the open list, we compute a finite - heuristic value. The second time we insert it, the cached value is reused. - - During first expansion, the heuristic value is recomputed and might become - infinite, for example because the reevaluation uses a stronger heuristic or - because the heuristic is path-dependent and we have accumulated more - information in the meantime. Then upon second expansion we have a dead-end - node which we must ignore. + For example, consider a state s that is reached twice before it is + expanded. The first time we insert it into the open list, we + compute a finite heuristic value. The second time we insert it, + the cached value is reused. + + During first expansion, the heuristic value is recomputed and + might become infinite, for example because the reevaluation uses a + stronger heuristic or because the heuristic is path-dependent and + we have accumulated more information in the meantime. Then upon + second expansion we have a dead-end node which we must ignore. */ if (node->is_dead_end()) continue; if (lazy_evaluator->is_estimate_cached(s)) { int old_h = lazy_evaluator->get_cached_estimate(s); - int new_h = eval_context.get_evaluator_value_or_infinity(lazy_evaluator.get()); + int new_h = eval_context.get_evaluator_value_or_infinity( + lazy_evaluator.get()); if (open_list->is_dead_end(eval_context)) { node->mark_as_dead_end(); statistics.inc_dead_ends(); @@ -190,7 +192,8 @@ SearchStatus EagerSearch::step() { // This evaluates the expanded state (again) to get preferred ops EvaluationContext eval_context(s, node->get_g(), false, &statistics, true); ordered_set::OrderedSet preferred_operators; - for (const shared_ptr &preferred_operator_evaluator : preferred_operator_evaluators) { + for (const shared_ptr &preferred_operator_evaluator : + preferred_operator_evaluators) { collect_preferred_operators(eval_context, preferred_operator_evaluator.get(), preferred_operators); @@ -224,8 +227,8 @@ SearchStatus EagerSearch::step() { // TODO: Make this less fragile. int succ_g = node->get_g() + get_adjusted_cost(op); - EvaluationContext succ_eval_context( - succ_state, succ_g, is_preferred, &statistics); + EvaluationContext succ_eval_context(succ_state, succ_g, + is_preferred, &statistics); statistics.inc_evaluated_states(); if (open_list->is_dead_end(succ_eval_context)) { @@ -233,7 +236,7 @@ SearchStatus EagerSearch::step() { statistics.inc_dead_ends(); continue; } - succ_node.open(*node, op, get_adjusted_cost(op)); + succ_node.open_new_node(*node, op, get_adjusted_cost(op)); open_list->insert(succ_eval_context, succ_state.get_id()); if (search_progress.check_progress(succ_eval_context)) { @@ -242,7 +245,7 @@ SearchStatus EagerSearch::step() { } } else if (succ_node.get_g() > node->get_g() + get_adjusted_cost(op)) { // We found a new cheapest path to an open or closed state. - if (reopen_closed_nodes) { + if (succ_node.is_open() || reopen_closed_nodes) { if (succ_node.is_closed()) { /* TODO: It would be nice if we had a way to test @@ -252,8 +255,12 @@ SearchStatus EagerSearch::step() { consistent heuristic). */ statistics.inc_reopened(); + succ_node.reopen_closed_node(*node, op, + get_adjusted_cost(op)); + } else { + succ_node.update_open_node_parent(*node, op, + get_adjusted_cost(op)); } - succ_node.reopen(*node, op, get_adjusted_cost(op)); EvaluationContext succ_eval_context( succ_state, succ_node.get_g(), is_preferred, &statistics); @@ -277,10 +284,11 @@ SearchStatus EagerSearch::step() { */ open_list->insert(succ_eval_context, succ_state.get_id()); } else { - // If we do not reopen closed nodes, we just update the parent pointers. - // Note that this could cause an incompatibility between - // the g-value and the actual path that is traced back. - succ_node.update_parent(*node, op, get_adjusted_cost(op)); + // If we do not reopen closed nodes, we just update the parent + // pointers. Note that this could cause an incompatibility + // between the g-value and the actual path that is traced back. + succ_node.update_open_node_parent(*node, op, + get_adjusted_cost(op)); } } } diff --git a/src/search/search_algorithms/enforced_hill_climbing_search.cc b/src/search/search_algorithms/enforced_hill_climbing_search.cc index 3899753adb..a8c1b3b945 100644 --- a/src/search/search_algorithms/enforced_hill_climbing_search.cc +++ b/src/search/search_algorithms/enforced_hill_climbing_search.cc @@ -214,7 +214,8 @@ SearchStatus EnforcedHillClimbingSearch::ehc() { } int h = eval_context.get_evaluator_value(evaluator.get()); - node.open(parent_node, last_op, get_adjusted_cost(last_op)); + node.open_new_node(parent_node, last_op, + get_adjusted_cost(last_op)); if (h < current_eval_context.get_evaluator_value(evaluator.get())) { ++num_ehc_phases; diff --git a/src/search/search_algorithms/lazy_search.cc b/src/search/search_algorithms/lazy_search.cc index 6b9870217a..b4f933ceac 100644 --- a/src/search/search_algorithms/lazy_search.cc +++ b/src/search/search_algorithms/lazy_search.cc @@ -183,10 +183,13 @@ SearchStatus LazySearch::step() { SearchNode parent_node = search_space.get_node(parent_state); OperatorProxy current_operator = task_proxy.get_operators()[current_operator_id]; if (reopen) { - node.reopen(parent_node, current_operator, get_adjusted_cost(current_operator)); + node.reopen_closed_node(parent_node, current_operator, + get_adjusted_cost( + current_operator)); statistics.inc_reopened(); } else { - node.open(parent_node, current_operator, get_adjusted_cost(current_operator)); + node.open_new_node(parent_node, current_operator, + get_adjusted_cost(current_operator)); } } node.close(); diff --git a/src/search/search_space.cc b/src/search/search_space.cc index 7ad271632f..c235b2412f 100644 --- a/src/search/search_space.cc +++ b/src/search/search_space.cc @@ -53,44 +53,36 @@ void SearchNode::open_initial() { info.creating_operator = OperatorID::no_operator; } -void SearchNode::open(const SearchNode &parent_node, - const OperatorProxy &parent_op, - int adjusted_cost) { - assert(info.status == SearchNodeInfo::NEW); - info.status = SearchNodeInfo::OPEN; +void SearchNode::update_parent(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost) { info.g = parent_node.info.g + adjusted_cost; info.real_g = parent_node.info.real_g + parent_op.get_cost(); info.parent_state_id = parent_node.get_state().get_id(); info.creating_operator = OperatorID(parent_op.get_id()); } -void SearchNode::reopen(const SearchNode &parent_node, +void SearchNode::open_new_node(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost) { + assert(info.status == SearchNodeInfo::NEW); + info.status = SearchNodeInfo::OPEN; + update_parent(parent_node, parent_op, adjusted_cost); +} + +void SearchNode::reopen_closed_node(const SearchNode &parent_node, const OperatorProxy &parent_op, int adjusted_cost) { - assert(info.status == SearchNodeInfo::OPEN || - info.status == SearchNodeInfo::CLOSED); - - // The latter possibility is for inconsistent heuristics, which - // may require reopening closed nodes. + assert(info.status == SearchNodeInfo::CLOSED); info.status = SearchNodeInfo::OPEN; - info.g = parent_node.info.g + adjusted_cost; - info.real_g = parent_node.info.real_g + parent_op.get_cost(); - info.parent_state_id = parent_node.get_state().get_id(); - info.creating_operator = OperatorID(parent_op.get_id()); + update_parent(parent_node, parent_op, adjusted_cost); } -// like reopen, except doesn't change status -void SearchNode::update_parent(const SearchNode &parent_node, +void SearchNode::update_open_node_parent(const SearchNode &parent_node, const OperatorProxy &parent_op, int adjusted_cost) { - assert(info.status == SearchNodeInfo::OPEN || - info.status == SearchNodeInfo::CLOSED); - // The latter possibility is for inconsistent heuristics, which - // may require reopening closed nodes. - info.g = parent_node.info.g + adjusted_cost; - info.real_g = parent_node.info.real_g + parent_op.get_cost(); - info.parent_state_id = parent_node.get_state().get_id(); - info.creating_operator = OperatorID(parent_op.get_id()); + assert(info.status == SearchNodeInfo::OPEN); + update_parent(parent_node, parent_op, adjusted_cost); } void SearchNode::close() { diff --git a/src/search/search_space.h b/src/search/search_space.h index 1b2802b21d..43c9a3c32b 100644 --- a/src/search/search_space.h +++ b/src/search/search_space.h @@ -18,6 +18,10 @@ class LogProxy; class SearchNode { State state; SearchNodeInfo &info; + + void update_parent(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost); public: SearchNode(const State &state, SearchNodeInfo &info); @@ -32,15 +36,15 @@ class SearchNode { int get_real_g() const; void open_initial(); - void open(const SearchNode &parent_node, - const OperatorProxy &parent_op, - int adjusted_cost); - void reopen(const SearchNode &parent_node, - const OperatorProxy &parent_op, - int adjusted_cost); - void update_parent(const SearchNode &parent_node, + void open_new_node(const SearchNode &parent_node, const OperatorProxy &parent_op, int adjusted_cost); + void reopen_closed_node(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost); + void update_open_node_parent(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost); void close(); void mark_as_dead_end(); From 312df28bfd801c3544fb4c8faf160d390adee84f Mon Sep 17 00:00:00 2001 From: Remo Christen Date: Wed, 10 Jul 2024 00:36:19 +0200 Subject: [PATCH 2/8] [issue1140] Undo reformatting. --- src/search/search_algorithms/eager_search.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/search/search_algorithms/eager_search.cc b/src/search/search_algorithms/eager_search.cc index be135323b9..2322808f7c 100644 --- a/src/search/search_algorithms/eager_search.cc +++ b/src/search/search_algorithms/eager_search.cc @@ -155,8 +155,7 @@ SearchStatus EagerSearch::step() { if (lazy_evaluator->is_estimate_cached(s)) { int old_h = lazy_evaluator->get_cached_estimate(s); - int new_h = eval_context.get_evaluator_value_or_infinity( - lazy_evaluator.get()); + int new_h = eval_context.get_evaluator_value_or_infinity(lazy_evaluator.get()); if (open_list->is_dead_end(eval_context)) { node->mark_as_dead_end(); statistics.inc_dead_ends(); @@ -192,8 +191,7 @@ SearchStatus EagerSearch::step() { // This evaluates the expanded state (again) to get preferred ops EvaluationContext eval_context(s, node->get_g(), false, &statistics, true); ordered_set::OrderedSet preferred_operators; - for (const shared_ptr &preferred_operator_evaluator : - preferred_operator_evaluators) { + for (const shared_ptr &preferred_operator_evaluator : preferred_operator_evaluators) { collect_preferred_operators(eval_context, preferred_operator_evaluator.get(), preferred_operators); @@ -227,8 +225,8 @@ SearchStatus EagerSearch::step() { // TODO: Make this less fragile. int succ_g = node->get_g() + get_adjusted_cost(op); - EvaluationContext succ_eval_context(succ_state, succ_g, - is_preferred, &statistics); + EvaluationContext succ_eval_context( + succ_state, succ_g, is_preferred, &statistics); statistics.inc_evaluated_states(); if (open_list->is_dead_end(succ_eval_context)) { From 541c90e6fe04c094bdf6397225816b107b581262 Mon Sep 17 00:00:00 2001 From: Remo Christen Date: Wed, 10 Jul 2024 00:50:11 +0200 Subject: [PATCH 3/8] [issue1140] Fix style. --- src/search/search_space.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/search/search_space.cc b/src/search/search_space.cc index c235b2412f..aa677f2114 100644 --- a/src/search/search_space.cc +++ b/src/search/search_space.cc @@ -63,24 +63,24 @@ void SearchNode::update_parent(const SearchNode &parent_node, } void SearchNode::open_new_node(const SearchNode &parent_node, - const OperatorProxy &parent_op, - int adjusted_cost) { + const OperatorProxy &parent_op, + int adjusted_cost) { assert(info.status == SearchNodeInfo::NEW); info.status = SearchNodeInfo::OPEN; update_parent(parent_node, parent_op, adjusted_cost); } void SearchNode::reopen_closed_node(const SearchNode &parent_node, - const OperatorProxy &parent_op, - int adjusted_cost) { + const OperatorProxy &parent_op, + int adjusted_cost) { assert(info.status == SearchNodeInfo::CLOSED); info.status = SearchNodeInfo::OPEN; update_parent(parent_node, parent_op, adjusted_cost); } void SearchNode::update_open_node_parent(const SearchNode &parent_node, - const OperatorProxy &parent_op, - int adjusted_cost) { + const OperatorProxy &parent_op, + int adjusted_cost) { assert(info.status == SearchNodeInfo::OPEN); update_parent(parent_node, parent_op, adjusted_cost); } From d1c45cfe4b7b4bec54d24c21c4bc3b0a0644a2b6 Mon Sep 17 00:00:00 2001 From: Remo Christen Date: Fri, 12 Jul 2024 00:23:40 +0200 Subject: [PATCH 4/8] [issue1140] Flatten conditions and assume closed instead of open. --- src/search/search_algorithms/eager_search.cc | 45 +++++++++----------- src/search/search_space.cc | 7 +++ src/search/search_space.h | 3 ++ 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/src/search/search_algorithms/eager_search.cc b/src/search/search_algorithms/eager_search.cc index 2322808f7c..684ff3e930 100644 --- a/src/search/search_algorithms/eager_search.cc +++ b/src/search/search_algorithms/eager_search.cc @@ -243,26 +243,24 @@ SearchStatus EagerSearch::step() { } } else if (succ_node.get_g() > node->get_g() + get_adjusted_cost(op)) { // We found a new cheapest path to an open or closed state. - if (succ_node.is_open() || reopen_closed_nodes) { - if (succ_node.is_closed()) { - /* - TODO: It would be nice if we had a way to test - that reopening is expected behaviour, i.e., exit - with an error when this is something where - reopening should not occur (e.g. A* with a - consistent heuristic). - */ - statistics.inc_reopened(); - succ_node.reopen_closed_node(*node, op, - get_adjusted_cost(op)); - } else { - succ_node.update_open_node_parent(*node, op, - get_adjusted_cost(op)); - } - + if (succ_node.is_open()) { + succ_node.update_open_node_parent( + *node, op, get_adjusted_cost(op)); + EvaluationContext succ_eval_context( + succ_state, succ_node.get_g(), is_preferred, &statistics); + open_list->insert(succ_eval_context, succ_state.get_id()); + } else if (succ_node.is_closed() && reopen_closed_nodes) { + /* + TODO: It would be nice if we had a way to test + that reopening is expected behaviour, i.e., exit + with an error when this is something where + reopening should not occur (e.g. A* with a + consistent heuristic). + */ + statistics.inc_reopened(); + succ_node.reopen_closed_node(*node, op, get_adjusted_cost(op)); EvaluationContext succ_eval_context( succ_state, succ_node.get_g(), is_preferred, &statistics); - /* Note: our old code used to retrieve the h value from the search node here. Our new code recomputes it as @@ -282,15 +280,14 @@ SearchStatus EagerSearch::step() { */ open_list->insert(succ_eval_context, succ_state.get_id()); } else { - // If we do not reopen closed nodes, we just update the parent - // pointers. Note that this could cause an incompatibility - // between the g-value and the actual path that is traced back. - succ_node.update_open_node_parent(*node, op, - get_adjusted_cost(op)); + assert(succ_node.is_closed() && !reopen_closed_nodes); + succ_node.update_closed_node_parent( + *node, op, get_adjusted_cost(op)); } + } else { + // We found a more expensive path to an open or closed state. } } - return IN_PROGRESS; } diff --git a/src/search/search_space.cc b/src/search/search_space.cc index aa677f2114..800103719d 100644 --- a/src/search/search_space.cc +++ b/src/search/search_space.cc @@ -85,6 +85,13 @@ void SearchNode::update_open_node_parent(const SearchNode &parent_node, update_parent(parent_node, parent_op, adjusted_cost); } +void SearchNode::update_closed_node_parent(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost) { + assert(info.status == SearchNodeInfo::CLOSED); + update_parent(parent_node, parent_op, adjusted_cost); +} + void SearchNode::close() { assert(info.status == SearchNodeInfo::OPEN); info.status = SearchNodeInfo::CLOSED; diff --git a/src/search/search_space.h b/src/search/search_space.h index 43c9a3c32b..b7d4b0cf07 100644 --- a/src/search/search_space.h +++ b/src/search/search_space.h @@ -45,6 +45,9 @@ class SearchNode { void update_open_node_parent(const SearchNode &parent_node, const OperatorProxy &parent_op, int adjusted_cost); + void update_closed_node_parent(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost); void close(); void mark_as_dead_end(); From a3a1e7210e7976042725a2eb8db21750fed58e88 Mon Sep 17 00:00:00 2001 From: Remo Christen Date: Fri, 12 Jul 2024 00:31:19 +0200 Subject: [PATCH 5/8] [issue1140] Fix style. --- src/search/search_space.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/search/search_space.h b/src/search/search_space.h index b7d4b0cf07..01aca776ba 100644 --- a/src/search/search_space.h +++ b/src/search/search_space.h @@ -46,8 +46,8 @@ class SearchNode { const OperatorProxy &parent_op, int adjusted_cost); void update_closed_node_parent(const SearchNode &parent_node, - const OperatorProxy &parent_op, - int adjusted_cost); + const OperatorProxy &parent_op, + int adjusted_cost); void close(); void mark_as_dead_end(); From 3fbb505ec1ce033299162283d9c35610809355e1 Mon Sep 17 00:00:00 2001 From: Remo Christen Date: Tue, 16 Jul 2024 21:53:06 +0200 Subject: [PATCH 6/8] [issue1140] Add back g-value discrepancy comment. --- src/search/search_algorithms/eager_search.cc | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/search/search_algorithms/eager_search.cc b/src/search/search_algorithms/eager_search.cc index 684ff3e930..cd3a599351 100644 --- a/src/search/search_algorithms/eager_search.cc +++ b/src/search/search_algorithms/eager_search.cc @@ -217,12 +217,14 @@ SearchStatus EagerSearch::step() { continue; if (succ_node.is_new()) { - // We have not seen this state before. - // Evaluate and create a new node. + /* + We have not seen this state before. + Evaluate and create a new node. - // Careful: succ_node.get_g() is not available here yet, - // hence the stupid computation of succ_g. - // TODO: Make this less fragile. + Careful: succ_node.get_g() is not available here yet, + hence the stupid computation of succ_g. + TODO: Make this less fragile. + */ int succ_g = node->get_g() + get_adjusted_cost(op); EvaluationContext succ_eval_context( @@ -280,6 +282,11 @@ SearchStatus EagerSearch::step() { */ open_list->insert(succ_eval_context, succ_state.get_id()); } else { + /* + If we do not reopen closed nodes, we just update the parent + pointers. Note that this could cause an incompatibility + between the g-value and the actual path that is traced back. + */ assert(succ_node.is_closed() && !reopen_closed_nodes); succ_node.update_closed_node_parent( *node, op, get_adjusted_cost(op)); From 91f126983de08278952284be95416d050701028a Mon Sep 17 00:00:00 2001 From: Remo Christen Date: Thu, 18 Jul 2024 12:18:52 +0200 Subject: [PATCH 7/8] [issue1140] Remove outdated comment. --- src/search/search_algorithms/eager_search.cc | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/search/search_algorithms/eager_search.cc b/src/search/search_algorithms/eager_search.cc index cd3a599351..b6bcf5bb05 100644 --- a/src/search/search_algorithms/eager_search.cc +++ b/src/search/search_algorithms/eager_search.cc @@ -263,23 +263,6 @@ SearchStatus EagerSearch::step() { succ_node.reopen_closed_node(*node, op, get_adjusted_cost(op)); EvaluationContext succ_eval_context( succ_state, succ_node.get_g(), is_preferred, &statistics); - /* - Note: our old code used to retrieve the h value from - the search node here. Our new code recomputes it as - necessary, thus avoiding the incredible ugliness of - the old "set_evaluator_value" approach, which also - did not generalize properly to settings with more - than one evaluator. - - Reopening should not happen all that frequently, so - the performance impact of this is hopefully not that - large. In the medium term, we want the evaluators to - remember evaluator values for states themselves if - desired by the user, so that such recomputations - will just involve a look-up by the Evaluator object - rather than a recomputation of the evaluator value - from scratch. - */ open_list->insert(succ_eval_context, succ_state.get_id()); } else { /* From 677087419ef040a7369c62657d0dc1f12a6c7e17 Mon Sep 17 00:00:00 2001 From: Remo Christen Date: Thu, 18 Jul 2024 17:28:55 +0200 Subject: [PATCH 8/8] [issue1140] Fix comment. --- src/search/search_algorithms/eager_search.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/search/search_algorithms/eager_search.cc b/src/search/search_algorithms/eager_search.cc index b6bcf5bb05..951906fbcc 100644 --- a/src/search/search_algorithms/eager_search.cc +++ b/src/search/search_algorithms/eager_search.cc @@ -275,7 +275,10 @@ SearchStatus EagerSearch::step() { *node, op, get_adjusted_cost(op)); } } else { - // We found a more expensive path to an open or closed state. + /* + We found an equally or more expensive path to an open or closed + state. + */ } } return IN_PROGRESS;