Update axiom documentation #233
Conversation
…since landmark_cost_partitioning_heuristic does not support axioms.
| @@ -17,9 +17,9 @@ class LandmarkSumHeuristic : public LandmarkHeuristic { | |||
| int get_heuristic_value(const State &ancestor_state) override; | |||
| public: | |||
| LandmarkSumHeuristic( | |||
| tasks::AxiomHandlingType axioms, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| "conditional effects when using a LandmarkFactory " | ||
| "not supporting them"); | ||
| "yes except on tasks with conditional effects when " | ||
| "using a LandmarkFactory not supporting them"); |
There was a problem hiding this comment.
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.
|
Looks good to me |
Update documentation for axiom support and safety properties for heuristics that have the new axioms option.
Furthermore, removed the axioms option from landmark_cost_partitioning_heuristic since it does not support axioms.