Hi, I have a somewhat minor feature request or rather a suggestion regarding 
FD's 
AbstractTask interface.

Currently, the AbstractTask interface provides a lot of conceptually independent 
functionality at once. It is required to define the variables, operators, 
axioms, 
operator costs, initial state and goal of the task at the same time. I think it 
would make much more sense to also have seperate interfaces VariableSpace, 
OperatorSpace, AxiomSpace, etc. which each provide only a dedicated part of the 
interface. The current monolithic interface is not easy to extend. In my own 
fork 
dedicated to probabilistic planning, operators have multiple effects, but 
everything else stays the same. Yet, my probabilistic task interface 
ProbabilisticTask cannot inherit only part of AbstractTask, so I had to extract 
the common part of the interface as a base class to make this work. I don't like 
hacking into FD's most basic interfaces though, as every adaptation makes it 
ever 
more challenging to merge with Fast Downward, and I use the classical planning 
code quite a bit as a baseline in my experiments. With these more fine-grained 
interfaces, I could just inherit from VariableSpace, CostFunction etc. except 
OperatorSpace, which I replace with my own interface ProbabilisticOperatorSpace.

Using these new interfaces, one could also make the dedicated proxy classes have 
a 
reference to only the corresponding interface. Then my implementation could also 
use the proxy classes, expect for OperatorProxy and similar, without manual 
changes. I suppose these changes quite minimal and should not affect the rest of 
the codebase, but it makes the perhaps most fundamental interface of the planner 
more user-friendly.

| As a concrete example, PR2 needs its own representation of State for things 
| (partial states, mutable, etc, etc), but we want to avoid duplicate implementation 
| everywhere. So we ended up modifying the core to run off an interface, instead of the 
| built-in State representation:
|
| https://github.com/aibasel/downward/compare/main...QuMuLab:pr2-downward-core:main#diff-
| 2722a18defc815113505d07d85276b09fdbfa7826efcc92cc68928b4b0af63a5R555

That sounds a bit more involved than what I do / need. The state representation is mostly fine for my purposes, I just needed to change the type of the task member of State to AbstractTaskBase, where I created the following hierarchy:

                     AbstractTaskBase
                (e.g., get_num_variables)
                   /                  \
                  /                    \
                 /                      \
         AbstractTask                  ProbabilisticTask
(e.g., get_num_operator_effects) (e.g., get_num_operator_outcomes)

So AbstractTaskBase is kind of like an intersection between classical and probabilistic planning task functionality. Whenever something I wanted to re-use in FD's code could be rewritten to use AbstractTaskBase instead, I changed it accordingly so it also works with ProbabilisticTask. But that code depends on a full AbstractTask or (TaskProxy) in the first place and not a more fine-grained sub-interface seems like a design flaw to me.

It would be great if a more fine-grained hierarchy existed similar to the one above existed, but I understand the covariance problems, I think. If one had very fine-grained interfaces like VariableSpace and OperatorSpace, then one could not express "derives from VariableSpace and OperatorSpace" without introducing a dedicated inheritant, but this cannot be done for every combination, so one would have to settle for something like AbstractTaskBase above, where this smaller interface is hopefully useful to users.

Though, I feel like in a perfect world, the task representation would use composition over inheritance and would compose its sub-interfaces, i.e., AbstractTask = std::tuple<std::shared_ptr<VariableSpace>, std::shared_ptr<OperatorSpace>, ...>. With that design, one gets all the interface methods, but one can also easily modify the task, for example the cost function component (leading to less boilerplate like in CostAdaptedTask), check for identical sub-components (e.g., to avoid re-computing SuccessorGenerators), share sub-components with other tasks, etc. Every function that operates on both a VariableSpace and OperatorSpace could just accept a std::tuple<std::shared_ptr<VariableSpace>, std::shared_ptr<OperatorSpace>> or std::tuple<const VariableSpace&, const OperatorSpace&>, depending on ownership semantics. The obvious drawbacks of this design would be overhead due to seperation of data and thus loss of data locality. And of course the implementation of this would mean to touch almost the entire planner :-)

> The main thing I want to avoid is that you invest a lot of time into a patch 
that we end up not merging.

Ya, I think a reasonable pattern might be to have prob / FOND modifications try 
to stay as pure as possible, and then we semi-regularly sit down with someone on 
the FD team to see if there's anything worth pulling out as a patch. It would 
have the added benefit of expert eyes on what we're doing wrong with the way 
we're trying to interface (I'm sure most on the FD team would be thoroughly 
aghast at the sight of our own proxy implementation ;)).

No rush on this from our side, but @tkloessner may be working on a different 
timeline.

Concrete suggestions always welcome. The main thing I want to avoid is that you invest a lot of time into a patch that we end up not merging.

From my perspective, the value is a lot higher if a patch is useful for both of you than just one of you because that increases the likelihood that it will also be useful for others. A golden rule of abstract interfaces is that you need three users to get it right -- with vanilla Fast Downward, FOND and probabilistic planning, we would have that.

I think there may be the odd thing that is simple and reduces the amount of integration 
complexity. As a concrete example, PR2 needs its own representation of State for things 
(partial states, mutable, etc, etc), but we want to avoid duplicate implementation 
everywhere. So we ended up modifying the core to run off an interface, instead of the 
built-in State representation:

https://github.com/aibasel/downward/compare/main...QuMuLab:pr2-downward-core:main#diff-
2722a18defc815113505d07d85276b09fdbfa7826efcc92cc68928b4b0af63a5R555

I suspect @tkloessner and I have the same underlying motivation: minimize the rewrites 
needed on the FD core to achieve our task. We'll never get to 0, but the effort is 
certainly worth it :P.

When all these classes are polymorphic, you end up with quite complex covariance problems that tend to have no good solutions in C++, especially no good efficient ones. That's why we ended up with the current split into the monolithic low-level AbstractTask class vs. the user-facing proxy classes that talk about individual operators etc.

I'm skeptical it's possible to find a simple design here that is mergeable from a performance standpoint and at the same time designed in such a way that it can be used off the shelf for probabilistic planning and FOND planning without similar pain points to the current state.

So I think this would not be such a minor feature. But there's no harm in you (singular or plural) having a look and suggesting a patch if you think it wouldn't be such a big deal. And if you want to do this, discussing possible design aspects beforehand also sounds like a good idea, although I don't think I'd be able to participate. (Too many other things in the queue for the next weeks.)

This is a very similar path I've taken for FOND planning. Maybe it's worth finding 
a time to collectively hash out what a useful interface might be? Exact same 
desire on our side to minimize the difficulty staying up-to-date with the FD core 
as a base to build off of.