The LandmarkFactory class has too many things going on for an abstract base class. In this issue, we'd like to move all functionality that should be handled by specific LandmarkFactory implementations there.

This is part of meta issue987.

I agree.

Thanks. h^m and lm_exhaust get slightly slower, but for me, this is acceptable.

The results of the optimal configurations with the requested changes can be found at the same location:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-v1-optimal-2-issue990-base-issue990-v1-compare.html

Thomas, I just saw you pushed the fix for h^m. Do plan to run all configs again? If so, please also update the lm_exhaust config (see issue987).

I addressed Maltes comments and started the optimal experiment again with the suggested changes (hm with m=2 and hm_exhaust with lazy evaluation).

I reviewed the changes already and the experimental results look fine, so for me this is good to be merged. 

I'll add the suggested update/changes to the configs to the meta issue987 so it doesn't get lost.

I've left some small code comments, but am otherwise fine with the pull request. I didn't review it thoroughly, so I don't know if someone else should still take a closer look or if you're fine with it.


For the experimental results, the h^m(m=2) configuration actually runs h^m(m=1), so m=2 is still not tested. Can you add this?

Regarding developing a "standard" set of landmark configurations for other issues, I would also enable the lazy evaluation for the A* configuration with lm_exhaust, i.e., make it like (for example) the lm_hm configuration, with the only difference the evaluator. But this is just a minor point.


This is again the kind of issue where it would be useful to include landmark statistics (e.g. no. landmarks, no. conjunctive/disjunctive landmarks, no. orderings of different types) in the results. For example, then it would probably have been obvious that h^m(m=2) wasn't run (from the fact that the configuration has 0 conjunctive landmarks). But we can merge it without these statistics.

Thanks Salome for the review. Malte and/or Silvan, would you like to add anything, or can this issue be merged?

I took a look at the results and they look good to me. The error messages seem to be fine (I saw segfaults in caldera and organic synthesis, h^add overflow in organic synthesis, exitcode-248 in miconic-fulladl and mystery which is related to issue998, and oversized run.logs). The difference in h-values in trucks-strips is also a known issue from what I gathered discussing issue995 (nondeterministic invariant synthesis due to hitting the time limit).

So from my side this looks good to merge, but it probably wouldn't hurt to hear Malte and/or Silvan's opinion as well.

Here are the results of the requested experiments:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-v1-satisficing-2-issue990-base-issue990-v1-compare.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-v1-optimal-2-issue990-base-issue990-v1-compare.html

The new version seems to use more memory but is a bit faster (which is the contrary of what Silvan said in msg9863). Coverage is always more or less the same, and increases once by more than just 1 (+4 for seq-opt-bjolp-opt).

I don't see anything suspicious in the results (the initial h value difference in the optimal lm-exhaust configuration is in trucks).

In terms of performance, this looks good to merge. In terms of code quality, I trust Silvan's review.

I think the additional configurations that Silvan mentions would be useful simply to have a general set of configurations with good coverage for larger changes to the landmark code. Perhaps they are less necessary for this issue than for others, but I think having such a configuration set will be quite useful for our work on the landmark code in general; might as well start using it for this issue if this doesn't keep us up too much.

We should check the logs. This would rule out certain classes of bugs.

I have seen this error many times when working on our landmark cycle-covering project. It is very strange, because it always occurs for several tasks in the same experiment run, but when I rerun the experiment it often does not occur at all anymare. Therefore, there is some non-deterministic behaviour in the system and I'm not convinced it has to do with the loaded modules.

We discussed the results, found a few general issues related to landmarks that are unrelated to this issue (Salomé will open issues on the tracker), but also stumbled across the unexplained errors of your reports. Many runs crashed due to the right GLIBCXX version not being available. I think that you might have compiled the code with a GCC version different from what is available at runtime on the compute nodes. Do you load a module for GCC in your .bashrc (or profile)? If not, that could be the reason. Otherwise, we need to figure out what is going on... 

Except for those errors, the results are fine also for Malte and Saloḿe. However, to cover all code execution paths of the landmark code, Malte suggest to include the following 3 additional configs when re-running experiments:
- a satisficing configuration like lama-first (copied from the alias), but with pref=true in lm_rhw
- an optimal configuration like seq-opt-bjolp but with a different factory: instead of lm_merged(...), only use h_m with m=2
- an optimal configuration like seq-opt-bjolp but with opt=true (this requires an installed LP solver)

Thanks. So the new configurations use less memory but can sometimes be a little slower, with one exception of BJOLP where the change is quite visible, in pipesworld-notankage (see search time). In the satisficing results, search time also seems to increase slightly more than it decreases (I'm looking at the search time score), but nothing to worry about.

So if we can live with the two instances of pipesworld-notankage, I would suggest to merge this one *without* the latest change, as just discussed on Discord. 

Malte, maybe you could have a short look at the results to say if you disagree?

Here are the comparative reports:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-v1-optimal-issue990-base-issue990-v1-compare.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-v1-satisficing-issue990-base-issue990-v1-compare.html

Silvan, I didn't touch the computation of reasonable orders, it's still done by each landmark factory by calling the corresponding function of the abstract base class. I believe there are better options, but I have the impression this needs more discussion and won't touch it before we agree what to do with it.

What I changed in my latest commit was to introduce a landmark factory that turns the landmark graph into an acyclic one, as proposed by Malte in the daily meeting today.

Could you please also generate comparative reports? common_setup.py should contain a method called add_comparison_table_step() or similar. It is easier to see the changes then.

In addition to the places you mentioned, the new class also needs to have a proper documentation so that the auto-generated doc on fast-downward.org contains information on computing acyclic landmark graphs.

Regarding adding this class in this issue, I actually didn't want to suggest to do this, but rather my comment "Then we should consider to break the computation of orderings up and distribute them to the right factories." was intended to say "ok, so let's not introduce a factory for computing reasonable orderings, but make that code specific to the factories". But never mind, let's keep it it. We should just make sure that there is no overlap with issue995 that Salomé is working on.

As Silvan suggested, I ran an extended experiment that covers all landmark factories. The results can be found here:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-v1-optimal.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-v1-satisficing.html

There is one instance in the optimal lm_exhaust experiment where the initial h-value differs, but that's problem 22 of trucks and I assume this is due to the translator non-determinism. Apart from that, I don't see any issues.

Since then, I have continued and implemented a landmark factory that turns the landmark graph generated by another landmark factory into an acyclic one.

I started another set of experiments to see if this affects performance. If this is not the case, I still have to update all lmcount occurences in portfolios, aliases etc. to use the lm_acyclic landmark factory. Is there anything else that needs to be updated in this way (I assume existing experiment scripts are not changes)?

With use tags to tag revisions we want to test. That is, the "main" revision would be the revision from which you branch away, and it would be called "issue990-base". Then, whenever you want to run experiments, you tag the version you want to test by using tag "issue990-v1" etc. 

See http://www.fast-downward.org/ForDevelopers/Git for the correct workflow.

I missed that Silvan suggested to address these landmark factories in this issue. I'm fine with that as well.

In any case, I'll run experiments for all landmark factories.

Silvan, I don't understand what you mean with the convention for revisions, can you be a bit more specific?

The implementation of approximate_reasonable_orders in the LandmarkFactory class is indeed independent of all landmark generation methods, it just needs a landmark graph as input and adds reasonable orders between all pairs of simple fact landmarks that "interfere" with each other. The comment in the code implies that the technique is described by Hoffmann et al.

Note that this is the only place in the code where reasonable orders are generated (at least if you trust my abilities using grep). The RHW implementation only generates greedy_necessary and natural orders.

That said, does it make sense to open (one or two?) issue(s) that create(s) acyclic landmark graphs and adds reasonable orders as described by Hoffmann et al.?

Oh, and I think we wanted to run experiments for *all* factories, didn't we?

Btw., I think our convention for revisions is to use issue990-base for what you previously called "main" and issue990-v1, issue990-v2 etc. for all revisions of this issue.

Ah, I missed Malte's comment. Then we should consider to break the computation of orderings up and distribute them to the right factories.

I suggest to close this one. Then Salomé can start working on the updated code to move the options to the right factories and also issue993 can be started.

Adding reasonable orders as a generic concept doesn't make much sense to me; this requires specific reasoning methods that would generally be tied in some way to an approach for generating landmarks.

It may be the case that in our current LM factories, the reasonable order inference approaches are largely or completely independent of the landmark inference approaches, in which case it makes sense to break it up. But we should then make it clear that we implement specific reasonable ordering approaches, not a generic one. For example, a name like "reasonable_orders_rhw" makes much more sense than "reasonable_orders". Inferring reasonable orders is like selecting a pattern for PDB heuristics or a merge strategy for M&S: there are many possible ways it can be done.

Since we also decided to move the third bullet point of msg9836 to a separate issue, this issue is ready for reviewing (or merging, Silvan already reviewed it).

In today's daily, we discussed to create a landmark factory that generates an acyclic landmark graph and (this still needs confirmation) also one that adds reasonable orders.

Since all private functions that could be turned into free functions (see bullet point 1 of msg9836) are relevant for either of these, I suggest not to address that bullet point in this issue (the functions will be moved to the new landmark factory classes anyway).

The second bullet point in msg9836 has been addressed. The renaming of the branch also means that we require a new pull request, which can be found at

https://github.com/aibasel/downward/pull/20

In issue987, Silvan mentioned the following two in addition:
1. the same [this refers to making this only available in factories where it makes sense] applies for approximate_reasonable_orders and reasonable orders, and maybe also making the graph acyclic
2. let all individual factories be responsible for post-processing (removing orders, making acyclic etc.)

I recall that we weren't sure yet how we wanted to handle these things. If this is how we want to deal with these things, we can keep the code as it is: currently, all post-processing - including generation of reasonable orders and making the graph acyclic -- is done in the subclasses. However, this leads to unnecessary code duplication that we weren't sure if we wanted to have.

The following things are still open in this issue:

- Some of the private functions in LandmarkFactory neither access a member variable in LandmarkFactory nor are specific to the generation of a landmark graph. We'd like to turn them into free functions.
- The branch name and with it the reference to the branch name in the commit messages contain a hyphen that should not be there (issue-990 instead of issue990).
- We'd like to move (some of?) the command line options in LandmarkFactory to its subclasses and only where they make sense (if there are options that make sense everywhere, we'd like to handle these with an "add_options_to_parser" method).

It's correct that there are still things we'd like to do before we can close this issue. I just didn't manage to enter them here last night because it was already quite late (will do so now).

Correct me if I'm wrong, but I thought that we decided to do a few more things here, such turning some methods which don't need class members into functions. I also thought we wanted to figure out which factory needs which options, but I think we can do this elsewhere. See also my comment msg9830 in the meta issue.

Add link to meta issue.

The title of this issue is changed because we decided to make this issue more specific. issue993 is a follow up issue for this one.

I previously had a look at the code and left a few comments, all of which have been addressed in the mean time. For me, this looks good to be merged, but maybe someone else could have a quick look.

[This pull request](https://github.com/aibasel/downward/pull/19) is on a version where everything that uses the Exploration object has been moved to the subclasses of LandmarkFactory.

In a first step, the goal was to move the Exploration object without making any functional changes. This is achieved with [commit 8075a09](https://github.com/aibasel/downward/pull/19/commits/8075a09e8e6be0ec1442796d79448b07bbcf5c00).

[Commit 69694b4](https://github.com/aibasel/downward/pull/19/commits/69694b45c89fe3b464cac5a24974d0c772bcc81b) removes the Exploration object used by LandmarkFactoryMerged by keeping information from the individual landmark graphs instead of recomputing them. This leads to a functional change since hm achievers are now computed with the calc_achievers implementation that was in LandmarkFactoryHM also when embedded into a LandmarkFactoryMerged, whereas this was "overwritten" with calc_achievers of LandmarkFactory before (in the case of embedding hm into a merged lm factory). An experiment shows that this functional change does not affect behavior:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-2021-01-25-B-optimal.html

An experiment for a satisficing configuration is an additional indication that nothing changed so far:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue990-2021-01-25-A-satisficing.html

We'd like to clean up the LandmarkFactory classes in this issues: the abstract base class provides too much functionality that should be moved to the classes that inherit from it.