[issue1140] Reinsert open nodes into open list upon finding cheaper p…

…aths.

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

@@ -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;
 }
 

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;

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();

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() {

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();