[issue1201] Check whether a configuration guarantees completeness #283
Conversation
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This computes a logical OR, where we want an AND, I think. I recommend to use std::all_of here and in some places below.
There was a problem hiding this comment.
Tanja and I were not sure about this but went with OR because we thought that for the completeness of the configuration it is enough if one of the open lists is "safe", i.e. ensures that all non-dead-ends are considered.
There was a problem hiding this comment.
You're right. My knee-jerk reaction was wrong and I should've thought about this more carefully. But I think this method deserves a comment :)
I had some other minor comments while looking over this, see below.
| } | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
| return open_list->pruning_is_safe() && pruning_method->is_safe(); |
| EvaluationContext &eval_context) const = 0; | ||
| virtual bool pruning_is_safe() const { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Make this a pure virtual function, like the ones above?
There was a problem hiding this comment.
Same discussion as with is_complete of search_algorithm.h. For the moment I would keep it like this because the completeness check is not yet implemented for all open lists.
| exitcode = ExitCode::SEARCH_UNSOLVABLE; | ||
| } else { | ||
| exitcode = ExitCode::SEARCH_UNSOLVED_INCOMPLETE; | ||
| } |
There was a problem hiding this comment.
Can't it also be that the search ran out of time?
There was a problem hiding this comment.
Looks like it but I'm not completely sure. In that case it should be enough to extend the check to something like search_algorithm->is_complete() && search_finished_in_time, right? (I haven't yet looked into how to do the second part of the check.)
| virtual void save_plan_if_necessary(); | ||
| virtual bool is_complete() const { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This should also become a pure virtual function eventually, but I guess you want to minimize the diff until more algorithms are supported.
There was a problem hiding this comment.
I think we could also keep it as it is to have a default value for future search algorithms but I do not have a strong opinion on this. For the moment I would definitely keep it like this because the completeness check is not yet implemented for all search algorithms.
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
You're right. My knee-jerk reaction was wrong and I should've thought about this more carefully. But I think this method deserves a comment :)
I had some other minor comments while looking over this, see below.
| return false; | ||
| } | ||
| // Even if the first evaluator is unsafe we can still say that | ||
| // pruning_is_safe is true if (allow_unsafe_pruning is false and) at least |
There was a problem hiding this comment.
| // pruning_is_safe is true if (allow_unsafe_pruning is false and) at least | |
| // pruning is safe if (allow_unsafe_pruning is false and) at least |
| // Even if the first evaluator is unsafe we can still say that | ||
| // pruning is safe if (allow_unsafe_pruning is false and) at least | ||
| // one other evaluator is safe. | ||
| for (auto eval : evaluators) { |
There was a problem hiding this comment.
| for (auto eval : evaluators) { | |
| for (const shared_ptr<Evaluator> &evaluator : evaluators) |
to be consistent with e.g. get_path_dependent_evaluators in this file
and to have less updates on the reference counter of the smart pointers in evaluators.
| template<class Entry> | ||
| bool AlternationOpenList<Entry>::pruning_is_safe() const { | ||
| // If at least one of the sub-lists ensures that no solvable state is | ||
| // pruned we know that this also holds for AlternationOpenList. |
There was a problem hiding this comment.
| template<class Entry> | ||
| bool AlternationOpenList<Entry>::pruning_is_safe() const { | ||
| // If at least one of the sub-lists ensures that no solvable state is | ||
| // pruned we know that this also holds for AlternationOpenList. |
There was a problem hiding this comment.
3 participants
No description provided.