This is part of meta issue987.

Pull request: https://github.com/aibasel/downward/pull/51

The change log entry has been added in commit 23eef6a86921340e51eed713eef54deca5380123.

I think we forgot to add an entry to the change notes in CHANGES.md.

This was merged in commit 4f552c7231d0ce7ed230f72b4796162624559908.

Thanks everyone!

The results look good to me. I also don't see a reason not to merge.

In v6, we save a copy of the vector of facts that describe a landmark in the Landmark constructor. The results are here: 

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-satisficing-v6-compare.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-optimal-v6-compare.html

As we can see, this has a slightly positive effect on the landmark generation time. Apart from that, the results are pretty much identical to the baseline and I don't see a reason not to merge this.

What do you think?

Thanks for the reviews Florian and Salomé! We addressed all the suggested changes and implemented those fitting this issue. After doing so, I ran another experiment with more configurations than the last ones:

https://ai.dmi.unibas.ch/_tmp_files/buechner/issue999-optimal-v5-compare.html
https://ai.dmi.unibas.ch/_tmp_files/buechner/issue999-satisficing-v5-compare.html

I did not find any particular reasons against merging this as is. Probably the most concerning thing is the drop in coverage by 2 in optimal/lm_hm. It might have to do with the increased landmark_generation_time in this case. However, I'd rather open a new issue for refactoring the landmark_factory_hm because I believe there's a lot that can be improved there.

Any other thoughts and opinions?

Here are the results of the experiment:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-optimal-v4-compare.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-satisficing-v4-compare.html

Regarding Maltes last message: the running experiment includes v1 again and will also report landmark specific information, thanks for pointing this out!

The title (and scope) of this issue was changed from "implement classes for different types of landmarks" and now only covers a sub issue of the original issue, which has been moved to issue1023.

The current implementation is at a point where landmark and landmark node information have been separated successfully, and the experimental results looked promising (the final experiment is still running, so we need to wait until we can confirm this last part). We therefore decided to split this off of the original issue and review the current changes as the diff is already large enough.

I'll share the final results once they are available, but it wouldn't hurt if someone had time to look at the pull request in the mean time.

Similarly to other landmark issues, I think the experiments should also report things relevant to the landmark code, such as landmark generation time, numbers of landmarks and orderings etc. This can hopefully be copied from experiments from some of the other landmark issues we worked on recently.

No need to create a new report for this experiment, but perhaps in the next experiment we can also include this data for base and v1.

I agree that the runtime data looks good to proceed. Unlike other landmark issues we have had recently, the data looks largely like noise if you look into the per-domain results. Previously we often had small difference that were quite consistent across domains.

Pull request added to summary

In a first step, Clemens and I moved parts of the LandmarkNode class to a new Landmark class that contains all the functionality not related to the graph structure over landmarks and replaced occurences of the LandmarkNode class with the new Landmark class where it was *obviously* possible.

We ran a small experiment with one optimal and one satisficing configuration to make sure we detect possible issues early on:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-optimal-compare.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-satisficing-compare.html

As it seems to be usual with the landmark code, we get a slight improvement in one experiment and slightly worse results in the other. Most importantly, there doesn't seem to be a fundamental issue anywhere, so I suggest we keep going and care about experimental results when we are closer to having a code version we are happy with.

The next steps to get closer to such a version are:
- use Landmark instead of LandmarkNode where possible (also in the non-obvious cases)
- Redesign the way Landmarks and LandmarkNodes are created
- Decide where Landmarks live and how they can be accessed

Note that this is clearly not an exhaustive list to close this issue, it's just the immediate next steps.

Currently, a LandmarkGraph consists of instances of LandmarkNode. The LandmarkNode class has several members to denot the type of a landmark:
- disjunctive for a disjunction of facts
- conjunctive for a conjunction of facts
- it is referred to as simple if it consists of exactly one fact

In this issue, we'd like to implement an abstract base class for landmarks. The distinction of different types can then be made using different classes that inherit from the base class. Each node in the LandmarkGraph should refer to an instance of any concrete landmark implementation.

 which have members denoting whether a landmark is simple (refers to one fact which must become t