[issue1036] Refactor landmark progression to make it sound.

We implement valid landmark progression according to Büchner et al. (ICAPS
2023). With this, the previously incomplete LAMA becomes complete and also we
can now use reasonable landmark ordrings for optimal planning. The implemented
progression functions can be turned on or off by the following (boolean) command
line options (default on) of the landmark_sum and landmark_cost_partitioning
heuristic: prog_goal, prog_gn, and prog_r (for goal landmarks, greedy-necessary
orderings, and reasonable orderings, respectively). The changes require more
memory per state, but in general the performance of these heuristics is merely
unaffected.

src/search/landmarks/landmark_cost_assignment.cc

@@ -19,18 +19,17 @@ using namespace std;
 namespace landmarks {
 LandmarkCostAssignment::LandmarkCostAssignment(
     const vector<int> &operator_costs, const LandmarkGraph &graph)
-    : empty(), lm_graph(graph), operator_costs(operator_costs) {
+    : lm_graph(graph), operator_costs(operator_costs) {
 }
 
 const set<int> &LandmarkCostAssignment::get_achievers(
-    int lmn_status, const Landmark &landmark) const {
+    const Landmark &landmark, bool past) const {
     // Return relevant achievers of the landmark according to its status.
-    if (lmn_status == lm_not_reached)
-        return landmark.first_achievers;
-    else if (lmn_status == lm_needed_again)
+    if (past) {
         return landmark.possible_achievers;
-    else
-        return empty;
+    } else {
+        return landmark.first_achievers;
+    }
 }
 
 
@@ -44,23 +43,27 @@ LandmarkUniformSharedCostAssignment::LandmarkUniformSharedCostAssignment(
 
 
 double LandmarkUniformSharedCostAssignment::cost_sharing_h_value(
-    const LandmarkStatusManager &lm_status_manager) {
+    const LandmarkStatusManager &lm_status_manager,
+    const State &ancestor_state) {
     vector<int> achieved_lms_by_op(operator_costs.size(), 0);
     vector<bool> action_landmarks(operator_costs.size(), false);
 
     const LandmarkGraph::Nodes &nodes = lm_graph.get_nodes();
+    ConstBitsetView past =
+        lm_status_manager.get_past_landmarks(ancestor_state);
+    ConstBitsetView future =
+        lm_status_manager.get_future_landmarks(ancestor_state);
 
     double h = 0;
 
     /* First pass:
        compute which op achieves how many landmarks. Along the way,
        mark action landmarks and add their cost to h. */
     for (auto &node : nodes) {
-        int lmn_status =
-            lm_status_manager.get_landmark_status(node->get_id());
-        if (lmn_status != lm_reached) {
+        int id = node->get_id();
+        if (future.test(id)) {
             const set<int> &achievers =
-                get_achievers(lmn_status, node->get_landmark());
+                get_achievers(node->get_landmark(), past.test(id));
             if (achievers.empty())
                 return numeric_limits<double>::max();
             if (use_action_landmarks && achievers.size() == 1) {
@@ -90,11 +93,10 @@ double LandmarkUniformSharedCostAssignment::cost_sharing_h_value(
        an action landmark; decrease the counters accordingly
        so that no unnecessary cost is assigned to these landmarks. */
     for (auto &node : nodes) {
-        int lmn_status =
-            lm_status_manager.get_landmark_status(node->get_id());
-        if (lmn_status != lm_reached) {
+        int id = node->get_id();
+        if (future.test(id)) {
             const set<int> &achievers =
-                get_achievers(lmn_status, node->get_landmark());
+                get_achievers(node->get_landmark(), past.test(id));
             bool covered_by_action_lm = false;
             for (int op_id : achievers) {
                 assert(utils::in_bounds(op_id, action_landmarks));
@@ -118,10 +120,10 @@ double LandmarkUniformSharedCostAssignment::cost_sharing_h_value(
        count shared costs for the remaining landmarks. */
     for (const LandmarkNode *node : relevant_lms) {
         // TODO: Iterate over Landmarks instead of LandmarkNodes
-        int lmn_status =
-            lm_status_manager.get_landmark_status(node->get_id());
+        int id = node->get_id();
+        assert(future.test(id));
         const set<int> &achievers =
-            get_achievers(lmn_status, node->get_landmark());
+            get_achievers(node->get_landmark(), past.test(id));
         double min_cost = numeric_limits<double>::max();
         for (int op_id : achievers) {
             assert(utils::in_bounds(op_id, achieved_lms_by_op));
@@ -174,10 +176,16 @@ lp::LinearProgram LandmarkEfficientOptimalSharedCostAssignment::build_initial_lp
 }
 
 double LandmarkEfficientOptimalSharedCostAssignment::cost_sharing_h_value(
-    const LandmarkStatusManager &lm_status_manager) {
+    const LandmarkStatusManager &lm_status_manager,
+    const State &ancestor_state) {
     /* TODO: We could also do the same thing with action landmarks we
              do in the uniform cost partitioning case. */
 
+
+    ConstBitsetView past =
+        lm_status_manager.get_past_landmarks(ancestor_state);
+    ConstBitsetView future =
+        lm_status_manager.get_future_landmarks(ancestor_state);
     /*
       Set up LP variable bounds for the landmarks.
       The range of cost(lm_1) is {0} if the landmark is already
@@ -186,10 +194,10 @@ double LandmarkEfficientOptimalSharedCostAssignment::cost_sharing_h_value(
     */
     int num_cols = lm_graph.get_num_landmarks();
     for (int lm_id = 0; lm_id < num_cols; ++lm_id) {
-        if (lm_status_manager.get_landmark_status(lm_id) == lm_reached) {
-            lp.get_variables()[lm_id].upper_bound = 0;
-        } else {
+        if (future.test(lm_id)) {
             lp.get_variables()[lm_id].upper_bound = lp_solver.get_infinity();
+        } else {
+            lp.get_variables()[lm_id].upper_bound = 0;
         }
     }
 
@@ -207,10 +215,9 @@ double LandmarkEfficientOptimalSharedCostAssignment::cost_sharing_h_value(
     }
     for (int lm_id = 0; lm_id < num_cols; ++lm_id) {
         const Landmark &landmark = lm_graph.get_node(lm_id)->get_landmark();
-        int lm_status = lm_status_manager.get_landmark_status(lm_id);
-        if (lm_status != lm_reached) {
+        if (future.test(lm_id)) {
             const set<int> &achievers =
-                get_achievers(lm_status, landmark);
+                get_achievers(landmark, past.test(lm_id));
             if (achievers.empty())
                 return numeric_limits<double>::max();
             for (int op_id : achievers) {

src/search/landmarks/landmark_cost_assignment.h

@@ -1,6 +1,8 @@
 #ifndef LANDMARKS_LANDMARK_COST_ASSIGNMENT_H
 #define LANDMARKS_LANDMARK_COST_ASSIGNMENT_H
 
+#include "../task_proxy.h"
+
 #include "../lp/lp_solver.h"
 
 #include <set>
@@ -15,20 +17,20 @@ class LandmarkNode;
 class LandmarkStatusManager;
 
 class LandmarkCostAssignment {
-    const std::set<int> empty;
 protected:
     const LandmarkGraph &lm_graph;
     const std::vector<int> operator_costs;
 
-    const std::set<int> &get_achievers(int lmn_status,
-                                       const Landmark &landmark) const;
+    const std::set<int> &get_achievers(
+        const Landmark &landmark, bool past) const;
 public:
     LandmarkCostAssignment(const std::vector<int> &operator_costs,
                            const LandmarkGraph &graph);
     virtual ~LandmarkCostAssignment() = default;
 
     virtual double cost_sharing_h_value(
-        const LandmarkStatusManager &lm_status_manager) = 0;
+        const LandmarkStatusManager &lm_status_manager,
+        const State &ancestor_state) = 0;
 };
 
 class LandmarkUniformSharedCostAssignment : public LandmarkCostAssignment {
@@ -39,7 +41,8 @@ class LandmarkUniformSharedCostAssignment : public LandmarkCostAssignment {
                                         bool use_action_landmarks);
 
     virtual double cost_sharing_h_value(
-        const LandmarkStatusManager &lm_status_manager) override;
+        const LandmarkStatusManager &lm_status_manager,
+        const State &ancestor_state) override;
 };
 
 class LandmarkEfficientOptimalSharedCostAssignment : public LandmarkCostAssignment {
@@ -62,7 +65,8 @@ class LandmarkEfficientOptimalSharedCostAssignment : public LandmarkCostAssignme
         lp::LPSolverType solver_type);
 
     virtual double cost_sharing_h_value(
-        const LandmarkStatusManager &lm_status_manager) override;
+        const LandmarkStatusManager &lm_status_manager,
+        const State &ancestor_state) override;
 };
 }
 

src/search/landmarks/landmark_cost_partitioning_heuristic.cc

@@ -30,11 +30,6 @@ void LandmarkCostPartitioningHeuristic::check_unsupported_features(
     const plugins::Options &opts) {
     shared_ptr<LandmarkFactory> lm_graph_factory =
         opts.get<shared_ptr<LandmarkFactory>>("lm_factory");
-    if (lm_graph_factory->computes_reasonable_orders()) {
-        cerr << "Reasonable orderings should not be used for "
-             << "admissible heuristics." << endl;
-        utils::exit_with(utils::ExitCode::SEARCH_INPUT_ERROR);
-    }
 
     if (task_properties::has_axioms(task_proxy)) {
         cerr << "Cost partitioning does not support axioms." << endl;
@@ -65,10 +60,11 @@ void LandmarkCostPartitioningHeuristic::set_cost_assignment(
 }
 
 int LandmarkCostPartitioningHeuristic::get_heuristic_value(
-    const State & /*state*/) {
+    const State &ancestor_state) {
     double epsilon = 0.01;
 
-    double h_val = lm_cost_assignment->cost_sharing_h_value(*lm_status_manager);
+    double h_val = lm_cost_assignment->cost_sharing_h_value(
+        *lm_status_manager, ancestor_state);
     if (h_val == numeric_limits<double>::max()) {
         return DEAD_END;
     } else {

src/search/landmarks/landmark_cost_partitioning_heuristic.h

@@ -12,7 +12,7 @@ class LandmarkCostPartitioningHeuristic : public LandmarkHeuristic {
     void check_unsupported_features(const plugins::Options &opts);
     void set_cost_assignment(const plugins::Options &opts);
 
-    int get_heuristic_value(const State &state) override;
+    int get_heuristic_value(const State &ancestor_state) override;
 public:
     explicit LandmarkCostPartitioningHeuristic(const plugins::Options &opts);
 

src/search/landmarks/landmark_heuristic.cc

@@ -8,12 +8,13 @@
 #include "../task_utils/successor_generator.h"
 #include "../tasks/cost_adapted_task.h"
 #include "../tasks/root_task.h"
+#include "../utils/markup.h"
 
 using namespace std;
 
 namespace landmarks {
 static bool landmark_is_interesting(
-    const State &state, const BitsetView &reached,
+    const State &state, ConstBitsetView &past,
     const landmarks::LandmarkNode &lm_node, bool all_lms_reached) {
     /*
       We consider a landmark interesting in two (exclusive) cases:
@@ -26,10 +27,10 @@ static bool landmark_is_interesting(
         const Landmark &landmark = lm_node.get_landmark();
         return landmark.is_true_in_goal && !landmark.is_true_in_state(state);
     } else {
-        return !reached.test(lm_node.get_id()) &&
+        return !past.test(lm_node.get_id()) &&
                all_of(lm_node.parents.begin(), lm_node.parents.end(),
                       [&](const pair<LandmarkNode *, EdgeType> parent) {
-                          return reached.test(parent.first->get_id());
+                          return past.test(parent.first->get_id());
                       });
     }
 }
@@ -56,8 +57,9 @@ void LandmarkHeuristic::initialize(const plugins::Options &opts) {
     }
 
     compute_landmark_graph(opts);
-    lm_status_manager =
-        utils::make_unique_ptr<LandmarkStatusManager>(*lm_graph);
+    lm_status_manager = utils::make_unique_ptr<LandmarkStatusManager>(
+        *lm_graph, opts.get<bool>("prog_goal"),
+        opts.get<bool>("prog_gn"), opts.get<bool>("prog_r"));
 
     if (use_preferred_operators) {
         /* Ideally, we should reuse the successor generator of the main
@@ -93,7 +95,7 @@ void LandmarkHeuristic::compute_landmark_graph(const plugins::Options &opts) {
 }
 
 void LandmarkHeuristic::generate_preferred_operators(
-    const State &state, const BitsetView &reached) {
+    const State &state, ConstBitsetView &past) {
     /*
       Find operators that achieve landmark leaves. If a simple landmark can be
       achieved, prefer only operators that achieve simple landmarks. Otherwise,
@@ -110,10 +112,10 @@ void LandmarkHeuristic::generate_preferred_operators(
     vector<OperatorID> preferred_operators_simple;
     vector<OperatorID> preferred_operators_disjunctive;
 
-    bool all_landmarks_reached = true;
-    for (int i = 0; i < reached.size(); ++i) {
-        if (!reached.test(i)) {
-            all_landmarks_reached = false;
+    bool all_landmarks_past = true;
+    for (int i = 0; i < past.size(); ++i) {
+        if (!past.test(i)) {
+            all_landmarks_past = false;
             break;
         }
     }
@@ -127,7 +129,7 @@ void LandmarkHeuristic::generate_preferred_operators(
             FactProxy fact_proxy = effect.get_fact();
             LandmarkNode *lm_node = lm_graph->get_node(fact_proxy.get_pair());
             if (lm_node && landmark_is_interesting(
-                    state, reached, *lm_node, all_landmarks_reached)) {
+                    state, past, *lm_node, all_landmarks_past)) {
                 if (lm_node->get_landmark().disjunctive) {
                     preferred_operators_disjunctive.push_back(op_id);
                 } else {
@@ -150,34 +152,42 @@ void LandmarkHeuristic::generate_preferred_operators(
 }
 
 int LandmarkHeuristic::compute_heuristic(const State &ancestor_state) {
-    State state = convert_ancestor_state(ancestor_state);
-    lm_status_manager->update_lm_status(ancestor_state);
-    int h = get_heuristic_value(state);
-
+    int h = get_heuristic_value(ancestor_state);
     if (use_preferred_operators) {
-        BitsetView reached_lms =
-            lm_status_manager->get_reached_landmarks(ancestor_state);
-        generate_preferred_operators(state, reached_lms);
+        ConstBitsetView past = lm_status_manager->get_past_landmarks(ancestor_state);
+        State state = convert_ancestor_state(ancestor_state);
+        generate_preferred_operators(state, past);
     }
-
     return h;
 }
 
 void LandmarkHeuristic::notify_initial_state(const State &initial_state) {
-    lm_status_manager->process_initial_state(initial_state, log);
+    lm_status_manager->progress_initial_state(initial_state);
 }
 
 void LandmarkHeuristic::notify_state_transition(
     const State &parent_state, OperatorID op_id, const State &state) {
-    lm_status_manager->process_state_transition(parent_state, op_id, state);
+    lm_status_manager->progress(parent_state, op_id, state);
     if (cache_evaluator_values) {
-        /* TODO:  It may be more efficient to check that the reached landmark
+        /* TODO:  It may be more efficient to check that the past landmark
             set has actually changed and only then mark the h value as dirty. */
         heuristic_cache[state].dirty = true;
     }
 }
 
 void LandmarkHeuristic::add_options_to_feature(plugins::Feature &feature) {
+    feature.document_synopsis(
+        "Landmark progression is implemented according to the following paper:"
+        + utils::format_conference_reference(
+            {"Clemens Büchner", "Thomas Keller", "Salomé Eriksson", "Malte Helmert"},
+            "Landmarks Progression in Heuristic Search",
+            "https://ai.dmi.unibas.ch/papers/buechner-et-al-icaps2023.pdf",
+            "Proceedings of the Thirty-Third International Conference on "
+            "Automated Planning and Scheduling (ICAPS 2023)",
+            "70-79",
+            "AAAI Press",
+            "2023"));
+
     feature.add_option<shared_ptr<LandmarkFactory>>(
         "lm_factory",
         "the set of landmarks to use for this heuristic. "
@@ -187,6 +197,14 @@ void LandmarkHeuristic::add_options_to_feature(plugins::Feature &feature) {
         "pref",
         "enable preferred operators (see note below)",
         "false");
+    /* TODO: Do we really want these options or should we just allways progress
+        everything we can? */
+    feature.add_option<bool>(
+        "prog_goal", "Use goal progression.", "true");
+    feature.add_option<bool>(
+        "prog_gn", "Use greedy-necessary ordering progression.", "true");
+    feature.add_option<bool>(
+        "prog_r", "Use reasonable ordering progression.", "true");
     Heuristic::add_options_to_feature(feature);
 
     feature.document_property("preferred operators",

src/search/landmarks/landmark_heuristic.h

@@ -3,7 +3,7 @@
 
 # include "../heuristic.h"
 
-class BitsetView;
+class ConstBitsetView;
 
 namespace successor_generator {
 class SuccessorGenerator;
@@ -26,22 +26,10 @@ class LandmarkHeuristic : public Heuristic {
     void initialize(const plugins::Options &opts);
     void compute_landmark_graph(const plugins::Options &opts);
 
-    /*
-      Unlike most landmark-related code, this function takes the
-      task-transformation of the state, not the original one (i.e., not
-      *ancestor_state*). This is because updating the landmark status manager
-      happens in *compute_heuristic(...)* before *get_heuristic_value(...)*
-      is called. Here, we only compute a heuristic value based on the
-      information in the landmark status manager, which does not require the
-      state at this point. The only reason we need this argument is to guarantee
-      goal-awareness of the LM-count heuristic which does not hold under the
-      current function used for progressing the landmark statuses. Checking
-      whether a state is a goal state requires the task-transformed state.
-    */
-    virtual int get_heuristic_value(const State &state) = 0;
+    virtual int get_heuristic_value(const State &ancestor_state) = 0;
 
     void generate_preferred_operators(
-        const State &state, const BitsetView &reached);
+        const State &state, ConstBitsetView &past);
     virtual int compute_heuristic(const State &ancestor_state) override;
 public:
     explicit LandmarkHeuristic(const plugins::Options &opts);