> In landmark_cost_partitioning_heuristic.cc we have an override of the default with the method
> that returns always true, i.e. the same as the default implementation of Evaluator.
>
> In case we want to keep the default implementation of Evaluator we should remove such
> methods from the concrete classes.

Right. It exists for historical reasons: "landmark_cost_partitioning_heuristic" and "landmark_sum_heuristic" are both split off from a common heuristic, and when the split happened, the code for this method for "landmark_cost_partitioning_heuristic" apparently simplified to "true", and whoever did the split at the time didn't realize that this is already the inherited behaviour.

For what it's worth, the implementatino of dead_ends_is_reliable for these two heuristics is very suspect. I don't think there is a good conceptual reason why the two heuristics should behave differently in this regard. This seems to be a holdover from the old broken progression code, but now both should progress correctly, and whether or not they are safe should only depend on whether the used landmark factories provide the required guarantees. There should be no difference between the two heuristics regarding safety, they should return infinity for exactly the same states.

In landmark_cost_partitioning_heuristic.cc we have an override of the default with the method that returns always true, i.e. the same as the default implementation of Evaluator.

In case we want to keep the default implementation of Evaluator we should remove such methods from the concrete classes.

We've wanted to get rid of the distinction between the current "Evaluator" and "Heuristic" classes for a long time because the things that are added by "Heuristic" don't really correspond well to what are conceptually the differences between heuristics and more general evaluators.

But of course we could reconsider the idea to get rid of "Heuristic".

In any case, I'm not violently opposed to making this function pure virtual, in the spirit of "explicit is better than implicit". I think this is primarily a matter of personal preference, so perhaps something worth discussing in a larger group to gather opinions.

It actually only happened for const evaluator. For the weighted evaluator it was thought about but the edge case wight=infinity was not considered.

I agree with this for 'pure' heuristics.
For heuristics wrapped in a basic evaluator this is different.
One can avoid thinking about this aspect for evaluators. And this happened for const and weighted evaluator.

One less invasive option would be to move the default implementation into heuristic.h/cc but having it pure virtual in evaluator.h/cc

Unsafe heuristics are very rare because heuristics are usually based on solving a simplified problem, and it's uncommon for them to not be complete for these simplified problems.

The only heuristics in the literature I know that are unsafe "by design" are cg() and cea(), and these are not independent but essentially the same heuristic. (cg() is a "poor man's version" of cea().)

We have other unsafe cases that arise due to parametrization (such as constant evaluators with infinity as the argument or weighted evaluators with a weight of infinity).

This is different from search algorithms where incomplete algorithms are not at all unusual.

I think the risk that someone forgets to consider this aspect when adding a new heuristic is mitigated by the fact that we require documenting if a heuristic is safe in the user documentation, so one cannot really avoid thinking about this aspect.

I am not concerned about the const(infinity) but about new unsafe evaluators that could come.
A pure virtual function would remind me as developer to think about the safety of the evaluator.
As a reviewer I would likely forget about the default implementation when someone would make a PR with a new heuristic that does not override it.

In my mental model I do not think a heuristic is safe, unless there is a reason why it is safe.
It surprised me in fastdownward that it was somewhat the other way around.

[Malte on discord]

I think the reason why this can remain undetected for a long time is because it's not necessarily a big problem. Can you think of a realistic scenario where someone would use such an evaluator and run into a problem?
This doesn't mean the implementation shouldn't be fixed, of course.
But I think it would be worse if we claimed that using a constant evaluator like "1" means the search is incomplete (the behaviour we would get if the default implementation returned "true") because I think finite constant evaluators are the more common use case.
We can think about making the function pure virtual, but I'm not so concerned about this bug that I'm convinced it's the right choice. It adds a lot of boilerplate because it's unusual for an evaluator to not be safe.

The base class Evaluator implements the check `dead_ends_are_reliable()` (in issue1201 we rename it to `is_safe()`) with a default implementation `return true;`.
This was not overridden for the ConstEvaluator even though the user is allowed to call it as `const(infinity)`.
Allowing infinity for the ConstEvaluator makes it unsafe i.e. its dead ends are not reliable.
The default implementation of `dead_ends_are_reliable()` from the base class should be overridden in the ConstEvaluator to return false if the value is infinity.
I think we should either have the default implementation in the base class as false to not incorrectly claim that a heuristic/evaluator is safe.
Or we make `dead_ends_are_reliable` pure virtual in the base class.
I think the default implementation provokes edge cases like the one in the ConstEvaluator that could stay undetected for years and make the planner unsound in unexpected ways.