Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue1089 #168

Merged
merged 4 commits into from
Aug 4, 2023
Merged

Issue1089 #168

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a8049d
Drop generation of obedient-reasonable orderings.
ClemensBuechner Aug 2, 2023
415b4b3
Remove obedient-reasonable ordering type.
ClemensBuechner Aug 2, 2023
70688a0
Adapt log-message.
ClemensBuechner Aug 2, 2023
0bea7ea
Condense document note.
ClemensBuechner Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/search/landmarks/landmark_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ void LandmarkFactory::edge_add(LandmarkNode &from, LandmarkNode &to,
*/
assert(&from != &to);

if (type == EdgeType::REASONABLE || type == EdgeType::OBEDIENT_REASONABLE) { // simple cycle test
if (type == EdgeType::REASONABLE) { // simple cycle test
if (from.parents.find(&to) != from.parents.end()) { // Edge in opposite direction exists
if (log.is_at_least_debug()) {
log << "edge in opposite direction exists" << endl;
Expand Down Expand Up @@ -160,8 +160,7 @@ void LandmarkFactory::mk_acyclic_graph() {
// the old method for this is no longer available.
// assert(acyclic_node_set.size() == number_of_landmarks());
if (log.is_at_least_normal()) {
log << "Removed " << removed_edges
<< " reasonable or obedient reasonable orders" << endl;
log << "Removed " << removed_edges << " reasonable orders" << endl;
}
}

Expand All @@ -179,7 +178,7 @@ void LandmarkFactory::remove_first_weakest_cycle_edge(
to = next(it2)->first;
weakest_edge = edge;
}
if (weakest_edge == EdgeType::OBEDIENT_REASONABLE) {
if (weakest_edge == EdgeType::REASONABLE) {
break;
}
}
Expand Down
70 changes: 35 additions & 35 deletions src/search/landmarks/landmark_factory_reasonable_orders_hps.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,14 @@ void LandmarkFactoryReasonableOrdersHPS::generate_landmarks(const shared_ptr<Abs
if (log.is_at_least_normal()) {
log << "approx. reasonable orders" << endl;
}
approximate_reasonable_orders(task_proxy, false);
if (log.is_at_least_normal()) {
log << "approx. obedient reasonable orders" << endl;
}
approximate_reasonable_orders(task_proxy, true);

approximate_reasonable_orders(task_proxy);
mk_acyclic_graph();
}

void LandmarkFactoryReasonableOrdersHPS::approximate_reasonable_orders(
const TaskProxy &task_proxy, bool obedient_orders) {
const TaskProxy &task_proxy) {
/*
Approximate reasonable and obedient reasonable orders according
to Hoffmann et al. If flag "obedient_orders" is true, we
calculate obedient reasonable orders, otherwise reasonable
orders.
Approximate reasonable orders according to Hoffmann et al.

If node_p is in goal, then any node2_p which interferes with
node_p can be reasonably ordered before node_p. Otherwise, if
Expand All @@ -62,7 +54,7 @@ void LandmarkFactoryReasonableOrdersHPS::approximate_reasonable_orders(
if (landmark.is_true_in_state(initial_state))
return;

if (!obedient_orders && landmark.is_true_in_goal) {
if (landmark.is_true_in_goal) {
for (auto &node2_p : lm_graph->get_nodes()) {
const Landmark &landmark2 = node2_p->get_landmark();
if (landmark == landmark2 || landmark2.disjunctive)
Expand All @@ -84,11 +76,10 @@ void LandmarkFactoryReasonableOrdersHPS::approximate_reasonable_orders(
const EdgeType &edge = p.second;
if (parent_node.get_landmark().disjunctive)
continue;
if ((edge >= EdgeType::NATURAL || (obedient_orders && edge == EdgeType::REASONABLE)) &&
&parent_node != node_p.get()) {
if (edge >= EdgeType::NATURAL && &parent_node != node_p.get()) {
// find predecessors or parent and collect in "interesting nodes"
interesting_nodes.insert(&parent_node);
collect_ancestors(interesting_nodes, parent_node, obedient_orders);
collect_ancestors(interesting_nodes, parent_node);
}
}
}
Expand All @@ -100,10 +91,7 @@ void LandmarkFactoryReasonableOrdersHPS::approximate_reasonable_orders(
if (landmark == landmark2 || landmark2.disjunctive)
continue;
if (interferes(task_proxy, landmark2, landmark)) {
if (!obedient_orders)
edge_add(*node2_p, *node_p, EdgeType::REASONABLE);
else
edge_add(*node2_p, *node_p, EdgeType::OBEDIENT_REASONABLE);
edge_add(*node2_p, *node_p, EdgeType::REASONABLE);
}
}
}
Expand Down Expand Up @@ -221,8 +209,7 @@ bool LandmarkFactoryReasonableOrdersHPS::interferes(
}

void LandmarkFactoryReasonableOrdersHPS::collect_ancestors(
unordered_set<LandmarkNode *> &result,
LandmarkNode &node, bool use_reasonable) {
unordered_set<LandmarkNode *> &result, LandmarkNode &node) {
/* Returns all ancestors in the landmark graph of landmark node "start" */

// There could be cycles if use_reasonable == true
Expand All @@ -231,24 +218,21 @@ void LandmarkFactoryReasonableOrdersHPS::collect_ancestors(
for (const auto &p : node.parents) {
LandmarkNode &parent = *(p.first);
const EdgeType &edge = p.second;
if (edge >= EdgeType::NATURAL || (use_reasonable && edge == EdgeType::REASONABLE))
if (closed_nodes.count(&parent) == 0) {
open_nodes.push_back(&parent);
closed_nodes.insert(&parent);
result.insert(&parent);
}
if (edge >= EdgeType::NATURAL && closed_nodes.count(&parent) == 0) {
open_nodes.push_back(&parent);
closed_nodes.insert(&parent);
result.insert(&parent);
}
}
while (!open_nodes.empty()) {
LandmarkNode &node2 = *(open_nodes.front());
for (const auto &p : node2.parents) {
LandmarkNode &parent = *(p.first);
const EdgeType &edge = p.second;
if (edge >= EdgeType::NATURAL || (use_reasonable && edge == EdgeType::REASONABLE)) {
if (closed_nodes.count(&parent) == 0) {
open_nodes.push_back(&parent);
closed_nodes.insert(&parent);
result.insert(&parent);
}
if (edge >= EdgeType::NATURAL && closed_nodes.count(&parent) == 0) {
open_nodes.push_back(&parent);
closed_nodes.insert(&parent);
result.insert(&parent);
}
}
open_nodes.pop_front();
Expand Down Expand Up @@ -382,8 +366,7 @@ class LandmarkFactoryReasonableOrdersHPSFeature : public plugins::TypedFeature<L
LandmarkFactoryReasonableOrdersHPSFeature() : TypedFeature("lm_reasonable_orders_hps") {
document_title("HPS Orders");
document_synopsis(
"Adds reasonable orders and obedient reasonable orders "
"described in the following paper" +
"Adds reasonable orders described in the following paper" +
utils::format_journal_reference(
{"Jörg Hoffmann", "Julie Porteous", "Laura Sebastia"},
"Ordered Landmarks in Planning",
Expand All @@ -393,6 +376,23 @@ class LandmarkFactoryReasonableOrdersHPSFeature : public plugins::TypedFeature<L
"215-278",
"2004"));

document_note(
"Obedient-reasonable Orderings",
ClemensBuechner marked this conversation as resolved.
Show resolved Hide resolved
"Hoffmann et al. (2004) suggest obedient-reasonable orderings "
"in addition to reasonable orderings. Obedient-reasonable "
"orderings were later also used by the LAMA planner (Richter and "
"Westphal, 2010). They are \"reasonable orderings\" under the "
"assumption that the (non-obedient) reasonable orderings are "
"actually \"natural\", i.e., every plan obeys the reasonable "
"orderings. We observed experimentally that obedient-reasonable "
"orderings have minimal effect on the performance of LAMA when "
"working on the theory underlying issue1036 (Büchner et al., "
"2023). More specifically, we observed a slight decline in plan "
"quality. Furthermore, we are not aware of a valid progression "
"function according to that theory. Support for computing "
"obedient-reasonable orderings was therefore removed in issue1089, "
"as they were not used anywhere anymore.");

add_option<shared_ptr<LandmarkFactory>>("lm_factory");
add_landmark_factory_options_to_feature(*this);

Expand Down
5 changes: 2 additions & 3 deletions src/search/landmarks/landmark_factory_reasonable_orders_hps.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@ class LandmarkFactoryReasonableOrdersHPS : public LandmarkFactory {
virtual void generate_landmarks(const std::shared_ptr<AbstractTask> &task) override;

void approximate_reasonable_orders(
const TaskProxy &task_proxy, bool obedient_orders);
const TaskProxy &task_proxy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line break no longer needed.

bool interferes(
const TaskProxy &task_proxy, const Landmark &landmark_a,
const Landmark &landmark_b) const;
void collect_ancestors(
std::unordered_set<LandmarkNode *> &result,
LandmarkNode &node, bool use_reasonable);
std::unordered_set<LandmarkNode *> &result, LandmarkNode &node);
bool effect_always_happens(
const VariablesProxy &variables, const EffectsProxy &effects,
std::set<FactPair> &eff) const;
Expand Down
9 changes: 4 additions & 5 deletions src/search/landmarks/landmark_graph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,10 @@ enum class EdgeType {
special case of greedy-necessary, i.e., every necessary ordering is
greedy-necessary, but not vice versa.
*/
NECESSARY = 4,
GREEDY_NECESSARY = 3,
NATURAL = 2,
REASONABLE = 1,
OBEDIENT_REASONABLE = 0
NECESSARY = 3,
GREEDY_NECESSARY = 2,
NATURAL = 1,
REASONABLE = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could theoretically cause problems if the code refers to the number instead of the name anywhere, but I certainly hope this is not the case. I quickly grepped for 4/3/2/1/0 and did not notice anything (of course quite possible that I would have missed it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's a bit risky but I've talked about this with Malte and we think it's best to start the counting at 0. There's certainly some clauses like if (order_type >= EdgeType::NATURAL) but a simple shift does clearly not affect these cases.
Now we're at least two who tried to find further usages but weren't able to. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's never acceptable to use numbers instead of symbolic names for constants like these, so I wouldn't worry about the possibility that we do this anywhere in the code. If you want, we can also remove the numbers entirely and rely on the automatic numbers of enum values. But then we have to reverse the order, i.e., list them in the order REASONABLE, NATURAL, ... because numbering goes from 0 onwards.

};

class LandmarkNode {
Expand Down
3 changes: 0 additions & 3 deletions src/search/landmarks/util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,6 @@ static void dump_edge(int from, int to, EdgeType edge, utils::LogProxy &log) {
case EdgeType::REASONABLE:
cout << "\"r\", style=dashed";
break;
case EdgeType::OBEDIENT_REASONABLE:
cout << "\"o_r\", style=dashed";
break;
}
cout << "];\n";
}
Expand Down