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

Update axiom documentation #233

Merged
merged 5 commits into from
Oct 7, 2024
Merged

Update axiom documentation #233

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e430a0f
Update documentation for heuristics that now safely support axioms.
salome-eriksson Sep 17, 2024
56574b1
Move axioms option from landmark_heuristic to landmark_sum_heuristic …
salome-eriksson Sep 17, 2024
b7efd04
Change constructor order for landmark sum heuristic.
salome-eriksson Oct 1, 2024
4c07ed5
uncrustify
salome-eriksson Oct 1, 2024
ae8932f
Move axioms argument to the end of the constructor for landmark_sum.
salome-eriksson Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions src/search/heuristics/additive_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,11 @@ class AdditiveHeuristicFeature

document_language_support("action costs", "supported");
document_language_support("conditional effects", "supported");
document_language_support(
"axioms",
"supported (in the sense that the planner won't complain -- "
"handling of axioms might be very stupid "
"and even render the heuristic unsafe)");
document_language_support("axioms", "supported");

document_property("admissible", "no");
document_property("consistent", "no");
document_property("safe", "yes for tasks without axioms");
document_property("safe", "yes");
document_property("preferred operators", "yes");
}

Expand Down
6 changes: 1 addition & 5 deletions src/search/heuristics/cea_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,11 +458,7 @@ class ContextEnhancedAdditiveHeuristicFeature

document_language_support("action costs", "supported");
document_language_support("conditional effects", "supported");
document_language_support(
"axioms",
"supported (in the sense that the planner won't complain -- "
"handling of axioms might be very stupid "
"and even render the heuristic unsafe)");
document_language_support("axioms", "supported");

document_property("admissible", "no");
document_property("consistent", "no");
Expand Down
6 changes: 1 addition & 5 deletions src/search/heuristics/cg_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,11 +303,7 @@ class CGHeuristicFeature

document_language_support("action costs", "supported");
document_language_support("conditional effects", "supported");
document_language_support(
"axioms",
"supported (in the sense that the planner won't complain -- "
"handling of axioms might be very stupid "
"and even render the heuristic unsafe)");
document_language_support("axioms", "supported");

document_property("admissible", "no");
document_property("consistent", "no");
Expand Down
8 changes: 2 additions & 6 deletions src/search/heuristics/ff_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,11 @@ class FFHeuristicFeature

document_language_support("action costs", "supported");
document_language_support("conditional effects", "supported");
document_language_support(
"axioms",
"supported (in the sense that the planner won't complain -- "
"handling of axioms might be very stupid "
"and even render the heuristic unsafe)");
document_language_support("axioms", "supported");

document_property("admissible", "no");
document_property("consistent", "no");
document_property("safe", "yes for tasks without axioms");
document_property("safe", "yes");
document_property("preferred operators", "yes");
}

Expand Down
8 changes: 2 additions & 6 deletions src/search/heuristics/max_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,11 @@ class HSPMaxHeuristicFeature

document_language_support("action costs", "supported");
document_language_support("conditional effects", "supported");
document_language_support(
"axioms",
"supported (in the sense that the planner won't complain -- "
"handling of axioms might be very stupid "
"and even render the heuristic unsafe)");
document_language_support("axioms", "supported");

document_property("admissible", "yes for tasks without axioms");
document_property("consistent", "yes for tasks without axioms");
document_property("safe", "yes for tasks without axioms");
document_property("safe", "yes");
document_property("preferred operators", "no");
}

Expand Down
3 changes: 1 addition & 2 deletions src/search/landmarks/landmark_cost_partitioning_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,12 @@ namespace landmarks {
LandmarkCostPartitioningHeuristic::LandmarkCostPartitioningHeuristic(
const shared_ptr<LandmarkFactory> &lm_factory, bool pref,
bool prog_goal, bool prog_gn, bool prog_r,
tasks::AxiomHandlingType axioms,
const shared_ptr<AbstractTask> &transform, bool cache_estimates,
const string &description, utils::Verbosity verbosity,
CostPartitioningMethod cost_partitioning, bool alm,
lp::LPSolverType lpsolver)
: LandmarkHeuristic(
axioms, pref, transform, cache_estimates, description, verbosity) {
pref, transform, cache_estimates, description, verbosity) {
if (log.is_at_least_normal()) {
log << "Initializing landmark cost partitioning heuristic..." << endl;
}
Expand Down
1 change: 0 additions & 1 deletion src/search/landmarks/landmark_cost_partitioning_heuristic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ class LandmarkCostPartitioningHeuristic : public LandmarkHeuristic {
LandmarkCostPartitioningHeuristic(
const std::shared_ptr<LandmarkFactory> &lm_factory, bool pref,
bool prog_goal, bool prog_gn, bool prog_r,
tasks::AxiomHandlingType axioms,
const std::shared_ptr<AbstractTask> &transform,
bool cache_estimates, const std::string &description,
utils::Verbosity verbosity,
Expand Down
11 changes: 3 additions & 8 deletions src/search/landmarks/landmark_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@ using namespace std;

namespace landmarks {
LandmarkHeuristic::LandmarkHeuristic(
tasks::AxiomHandlingType axioms, bool use_preferred_operators,
bool use_preferred_operators,
const shared_ptr<AbstractTask> &transform, bool cache_estimates,
const string &description, utils::Verbosity verbosity)
: Heuristic(tasks::get_default_value_axioms_task_if_needed(
transform, axioms),
cache_estimates, description, verbosity),
: Heuristic(transform, cache_estimates, description, verbosity),
use_preferred_operators(use_preferred_operators),
successor_generator(nullptr) {
}
Expand Down Expand Up @@ -228,16 +226,14 @@ void add_landmark_heuristic_options_to_feature(
"prog_gn", "Use greedy-necessary ordering progression.", "true");
feature.add_option<bool>(
"prog_r", "Use reasonable ordering progression.", "true");
tasks::add_axioms_option_to_feature(feature);
add_heuristic_options_to_feature(feature, description);

feature.document_property("preferred operators",
"yes (if enabled; see ``pref`` option)");
}

tuple<shared_ptr<LandmarkFactory>, bool, bool, bool, bool,
tasks::AxiomHandlingType, shared_ptr<AbstractTask>, bool, string,
utils::Verbosity>
shared_ptr<AbstractTask>, bool, string, utils::Verbosity>
get_landmark_heuristic_arguments_from_options(
const plugins::Options &opts) {
return tuple_cat(
Expand All @@ -247,7 +243,6 @@ get_landmark_heuristic_arguments_from_options(
opts.get<bool>("prog_goal"),
opts.get<bool>("prog_gn"),
opts.get<bool>("prog_r")),
tasks::get_axioms_arguments_from_options(opts),
get_heuristic_arguments_from_options(opts));
}
}
4 changes: 1 addition & 3 deletions src/search/landmarks/landmark_heuristic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ class LandmarkHeuristic : public Heuristic {
virtual int compute_heuristic(const State &ancestor_state) override;
public:
LandmarkHeuristic(
tasks::AxiomHandlingType axioms,
bool use_preferred_operators,
const std::shared_ptr<AbstractTask> &transform,
bool cache_estimates, const std::string &description,
Expand All @@ -66,8 +65,7 @@ class LandmarkHeuristic : public Heuristic {
extern void add_landmark_heuristic_options_to_feature(
plugins::Feature &feature, const std::string &description);
extern std::tuple<std::shared_ptr<LandmarkFactory>, bool, bool, bool,
bool, tasks::AxiomHandlingType,
std::shared_ptr<AbstractTask>, bool, std::string,
bool, std::shared_ptr<AbstractTask>, bool, std::string,
utils::Verbosity>
get_landmark_heuristic_arguments_from_options(
const plugins::Options &opts);
Expand Down
16 changes: 9 additions & 7 deletions src/search/landmarks/landmark_sum_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@ static bool are_dead_ends_reliable(
}

LandmarkSumHeuristic::LandmarkSumHeuristic(
tasks::AxiomHandlingType axioms,
const shared_ptr<LandmarkFactory> &lm_factory, bool pref,
bool prog_goal, bool prog_gn, bool prog_r,
tasks::AxiomHandlingType axioms,
const shared_ptr<AbstractTask> &transform, bool cache_estimates,
const string &description, utils::Verbosity verbosity)
: LandmarkHeuristic(
axioms, pref, transform, cache_estimates,
description, verbosity),
pref,
tasks::get_default_value_axioms_task_if_needed(transform, axioms),
cache_estimates, description, verbosity),
dead_ends_reliable(
are_dead_ends_reliable(lm_factory, task_proxy)) {
if (log.is_at_least_normal()) {
Expand Down Expand Up @@ -142,6 +143,7 @@ class LandmarkSumHeuristicFeature
"127-177",
"2010"));

tasks::add_axioms_option_to_feature(*this);
add_landmark_heuristic_options_to_feature(
*this, "landmark_sum_heuristic");

Expand Down Expand Up @@ -185,21 +187,21 @@ class LandmarkSumHeuristicFeature
"conditional_effects",
"supported if the LandmarkFactory supports them; otherwise "
"ignored");
document_language_support("axioms", "ignored");
document_language_support("axioms", "supported");

document_property("admissible", "no");
document_property("consistent", "no");
document_property(
"safe",
"yes except on tasks with axioms or on tasks with "
"conditional effects when using a LandmarkFactory "
"not supporting them");
"yes except on tasks with conditional effects when "
"using a LandmarkFactory not supporting them");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically we still have a bug that could maybe? lead to unsafe estimates on tasks with axioms, namely the one reported in issue247 and issue442. The short version is that we can introduce incorrect greedy-necessary orderings when derived variables are present. Before updating the landmark progression this could lead to incorrect dead-ends. With the new progression method this doesn't happen anymore on the tasks reported in these issues, but we're not entirely sure if it's guaranteed to not happen anymore.

}

virtual shared_ptr<LandmarkSumHeuristic> create_component(
const plugins::Options &opts,
const utils::Context &) const override {
return plugins::make_shared_from_arg_tuples<LandmarkSumHeuristic>(
tasks::get_axioms_arguments_from_options(opts),
get_landmark_heuristic_arguments_from_options(opts));
}
};
Expand Down
2 changes: 1 addition & 1 deletion src/search/landmarks/landmark_sum_heuristic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ class LandmarkSumHeuristic : public LandmarkHeuristic {
int get_heuristic_value(const State &ancestor_state) override;
public:
LandmarkSumHeuristic(
tasks::AxiomHandlingType axioms,
Copy link
Contributor Author

@salome-eriksson salome-eriksson Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not happy about the order here because landmark heuristics always had the factory as first option which has no default value. Now we have axioms as first option here with a default value, meaning we cannot write landmark_sum(lm_rhw()) anymore and instead need to write landmark_sum(lm_factory=lm_rhw())

This is also the reason the tests fail at the moment, but before I change the parameter strings anywhere I'd be interested if someone comes up with a better option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that landmark_sum(lm_rhw()) is more user friendly than landmark_sum(lm_factory=lm_rhw()).

One issue is the parameter test that checks if the parameters in the cc constructor match the parameters in the documentation of the feature.

If we do tasks::add_axioms_option_to_feature(*this); add_landmark_heuristic_options_to_feature( *this, "landmark_sum_heuristic"); then the test expects the AxiomHandlingType first. If we do it the other way around it expects the AxiomHandlingType in the very end.

A third option would be to not call add_landmark_heuristic_options_to_feature( *this, "landmark_sum_heuristic");
but to implement add_landmark_sum_heuristic_options_to_feature( *this);
That does adds the options (that differ from Heuristic) one by one, as add_landmark_heuristic_options_to_feature does but in the order we prefer and with a call to tasks::add_axioms_option_to_feature at the right moment.

const std::shared_ptr<LandmarkFactory> &lm_factory, bool pref,
bool prog_goal, bool prog_gn, bool prog_r,
tasks::AxiomHandlingType axioms,
const std::shared_ptr<AbstractTask> &transform,
bool cache_estimates, const std::string &description,
utils::Verbosity verbosity);
Expand Down
4 changes: 2 additions & 2 deletions src/search/tasks/default_value_axioms_task.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,9 +421,9 @@ static plugins::TypedEnumPlugin<AxiomHandlingType> _enum_plugin({
{"approximate_negative_cycles",
"Overapproximate negated axioms for all derived variables which "
"have cyclic dependencies by setting an empty condition, "
"indicating the default value can always be achieved for free."
"indicating the default value can always be achieved for free. "
"For all other derived variables, the negated axioms are computed"
"exactly. Note that this can potentially lead to a combinatorial "
"explosion"}
"explosion."}
});
}
Loading