From e408c7d03f2319a4283d6dc5213fe7483684462e Mon Sep 17 00:00:00 2001 From: remochristen Date: Mon, 22 Jul 2024 18:56:27 +0200 Subject: [PATCH] [issue1140] Reinsert open nodes into open list upon finding cheaper paths. Functionally, this change only affects EagerSearch. Until now, upon finding a cheaper path to an open node, it was only reinserted into the open list if the reopen_closed flag was set to true. This caused the unintuitive behavior that an EagerSearch configuration equivalent to the astar plugin, except with reopen_closed set to false, combined with a consistent heuristic did not guarantee optimal solutions. We remedy this by reinserting independent of the reopen_closed flag when discovering a cheaper path to a node. At the same time we restructure functions in search_space.* which causes renaming in other search algorithms, but their behavior does not change. --- src/search/search_algorithms/eager_search.cc | 94 +++++++++---------- .../enforced_hill_climbing_search.cc | 3 +- src/search/search_algorithms/lazy_search.cc | 7 +- src/search/search_space.cc | 55 ++++++----- src/search/search_space.h | 21 +++-- 5 files changed, 93 insertions(+), 87 deletions(-) diff --git a/src/search/search_algorithms/eager_search.cc b/src/search/search_algorithms/eager_search.cc index 5ec760e577..951906fbcc 100644 --- a/src/search/search_algorithms/eager_search.cc +++ b/src/search/search_algorithms/eager_search.cc @@ -139,15 +139,16 @@ 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; @@ -216,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( @@ -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,49 +245,42 @@ 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_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(*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) { /* - 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. + 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); 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. + */ + assert(succ_node.is_closed() && !reopen_closed_nodes); + succ_node.update_closed_node_parent( + *node, op, get_adjusted_cost(op)); } + } else { + /* + We found an equally or more expensive path to an open or closed + state. + */ } } - return IN_PROGRESS; } 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..800103719d 100644 --- a/src/search/search_space.cc +++ b/src/search/search_space.cc @@ -53,44 +53,43 @@ 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, - const OperatorProxy &parent_op, - int adjusted_cost) { - assert(info.status == SearchNodeInfo::OPEN || - info.status == SearchNodeInfo::CLOSED); +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); +} - // The latter possibility is for inconsistent heuristics, which - // may require reopening closed nodes. +void SearchNode::reopen_closed_node(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost) { + 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, - 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()); +void SearchNode::update_open_node_parent(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost) { + assert(info.status == SearchNodeInfo::OPEN); + 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() { diff --git a/src/search/search_space.h b/src/search/search_space.h index 1b2802b21d..01aca776ba 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,18 @@ 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 update_closed_node_parent(const SearchNode &parent_node, + const OperatorProxy &parent_op, + int adjusted_cost); void close(); void mark_as_dead_end();