PR: https://github.com/aibasel/downward/pull/235

Thanks for the review Malte. I responded to your point on Github. I think there are two main parts to the issue: one is the cyclic dependency that we currently solve with a "neat trick", the other is the question about moving axiom evaluators into the task to be able to reuse them in cases where this is possible and to avoid having a global variable storing them.

Regarding the first point, I agree that axiom evaluators should not depend on the initial state of a task. They should only depend on the axiom default values (which happen to be the values in the initial state, but this is not relevant for the axiom evaluator). On a mathematical level, I see the partial order of dependencies like this:
  Axiom Default Values + Axioms < Axiom Evaluator < Task
But that doesn't fit our design because we don't have a class to represent a set of axioms independent of a task.I'm fine with leaving the "little trick" as it is right now and taking this part out of the issue. The title is "Get rid of g_axiom_evaluators" which is focused on the second part anyway.

Regarding the second part: I do see advantages in the new solution despite the problems you mentioned in the pull requests. How should we proceed there? In the PR you mentioned discussing the design. Should we do this offline?

I cannot comment on the last message without looking at this code more.

I left some comments on the pull request.

On the general points you raise:

My gut feeling is that having to implement the axiom evaluator in terms of the low level interface would be worse than what we currently have.

"if we expect TaskProxy::get_initial_state() to return a state with evaluated axioms, then the code that evaluates the axioms shouldn't depend on TaskProxy"

That's not axiomatic. (Pun not originally intended.) The code that evaluates the axioms shouldn't depend on TaskProxy::get_initial_state().

AxiomEvaluator could be a "close friend" of the task that gets to use a task proxy whose get_initial_state method it is not allowed to call. This could be guarded/explained by comments and perhaps some additional assertions that make sure there is no foul play.

More broadly, there is a conceptual cyclic dependency here between the classes, so something has to give. A general way to break a cyclic dependency is to introduce another entity so that instead of the dependencies A -> B and B -> A we can have A -> B and B -> A'. (Using the raw task instead of the proxy in one place is an example of this.) Another possible solution would be to have two kinds of task proxies: ones with an initial state and ones without. Something like this could be done "physically" in the classes (two actually different classes) or via the state of the objects (an axiom evaluator can work on a task that happens to have no defined initial state). I think the second solution is more pragmatic.

But as always with axiom evaluators, there may be further things to be considered that I don't have on the radar. [Indeed we talked about this after I wrote this and before I could send it, and this seems to be the case.]

I also just saw this comment in DefaultValueAxiomsTask:

> - Technically, this transformation is illegal since it adds axioms which set
>  derived variables to their default value. Do not use it for search; the
>  axiom evaluator expects that all axioms set variables to their non-default
>  values (checked with an assertion).

I'm confused how this ever worked because if the heuristic accesses the initial state of this task (in the old code), wouldn't this need to have the axioms evaluated and thus trigger the assertion in the axiom evaluator? Anyway, we could make it explicitly illegal to use this transformation in a context that requires an axiom evaluator by overriding the method get_axiom_evaluator to raise an error instead of storing one locally.

I created a pull request to discuss the changes. It still has the hack of using the incomplete task in the axiom evaluator but already stores the axiom evaluator inside the task and delegates the getter where possible.

I would like to revive a discussion about the axiom evaluator, we had in issue774 ("Get rid of g_axiom_evaluator"). Back then, we moved from a single global axiom evaluator (g_axiom_evaluator) to a global hash map mapping tasks to axiom evaluators (g_axiom_evaluators).

As part of this change, we discussed different alternatives like storing axiom evaluators in a their task or storing a global map as we ended up doing. The argument back then was that we have multiple structures associated to a task that are difficult to compute and in some cases optional (axiom evaluator, successor generator, causal graph, int packer, landmarks). They should be computed on demand but only once per task. Back then, we aimed for a generic solution that fits all of these use cases and ended up with the PerTaskInformation that can store information for each task, makes sure it's computed only once per task and deletes the data if the task is deleted.

But we also saw that this was not perfect solution and some aspects would require more thought. The axiom evaluator is more closely linked to the task then, say, landmarks. The int packer belongs in the registry, and the successor generator should maybe be its own thing and not so integrated into the code of states and tasks. So in the end, it might not be a good solution to go for a uniform solution for these different parts.

In this issue I'm just focusing on the axiom evaluator and want to discuss if it makes sense to move it into the task as we had it in some intermediate version of issue774. One advantage I see is that currently, we map tasks to evaluators, for example a CostAdaptedTask that only changes the cost would get its own axiom evaluator. If we store it in the root task, and have the DelegatingTask forward the getter to its parent by default, then most of our task transformations (CostAdaptedTask, ModifiedGoalsTask, ModifiedOperatorCostsTask) could reuse the axiom evaluator of their parent task. The DomainAbstractedTask doesn't support axioms anyway, and the only remaining task transformation DefaultValueAxiomsTask would override the getter and create its own axiom evaluator (stored in the DefaultValueAxiomsTask).

There is one issue with storing the axiom evaluator inside an AbstractTask but we already have that issue now: the axiom evaluator would like to access the task using the high-level interface that TaskProxy provides but would be constructed in the constructor of an AbstractTask where TaskProxies and even the low-level interface of the abstract tasks are not ready yet. Currently, we use a TaskProxy anyway, relying on the axiom evaluator only accessing already constructed parts of the partially constructed task. This is obviously an ugly hack. In issue774 we deferred this as "still requires further thought". Maybe one solution could be to change the axiom evaluator in a way where it gets the task data not from an incomplete task but in a more basic form (vectors of default values, axiom rules, etc.) It is definitely nicer to use a TaskProxy but if we expect TaskProxy::get_initial_state() to return a state with evaluated axioms, then the code that evaluates the axioms shouldn't depend on TaskProxy.