Title Move functionality of the LandmarkFactory class to its subclasses
Priority wish Status resolved
Superseder Nosy List clemens, malte, salome, silvan, thomas
Assigned To thomas Keywords
Optional summary
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.

Created on 2021-01-22.12:06:30 by thomas, last changed by thomas.

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.
msg9976 (view) Author: malte Date: 2021-02-02.13:00:39
I agree.
msg9975 (view) Author: silvan Date: 2021-02-02.08:52:43
Thanks. h^m and lm_exhaust get slightly slower, but for me, this is acceptable.
msg9974 (view) Author: thomas Date: 2021-02-02.08:39:34
The results of the optimal configurations with the requested changes can be found at the same location:
msg9968 (view) Author: silvan Date: 2021-02-01.11:05:18
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).
msg9967 (view) Author: thomas Date: 2021-02-01.11:04:48
I addressed Maltes comments and started the optimal experiment again with the suggested changes (hm with m=2 and hm_exhaust with lazy evaluation).
msg9966 (view) Author: silvan Date: 2021-02-01.11:03:26
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.
msg9960 (view) Author: malte Date: 2021-01-30.17:43:22
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.
msg9952 (view) Author: thomas Date: 2021-01-29.17:33:11
Thanks Salome for the review. Malte and/or Silvan, would you like to add anything, or can this issue be merged?
msg9941 (view) Author: salome Date: 2021-01-29.12:47:57
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.
msg9937 (view) Author: thomas Date: 2021-01-29.10:35:35
Here are the results of the requested experiments:

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).
msg9907 (view) Author: malte Date: 2021-01-28.19:34:54
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.
msg9905 (view) Author: malte Date: 2021-01-28.19:27:20
We should check the logs. This would rule out certain classes of bugs.
msg9880 (view) Author: clemens Date: 2021-01-28.13:26:11
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.
msg9875 (view) Author: silvan Date: 2021-01-28.11:56:57
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)
msg9863 (view) Author: silvan Date: 2021-01-27.18:31:02
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?
msg9862 (view) Author: thomas Date: 2021-01-27.18:17:24
Here are the comparative reports:
msg9861 (view) Author: thomas Date: 2021-01-27.18:12:32
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.
msg9860 (view) Author: silvan Date: 2021-01-27.18:06:04
Could you please also generate comparative reports? 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 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.
msg9859 (view) Author: thomas Date: 2021-01-27.17:53:58
As Silvan suggested, I ran an extended experiment that covers all landmark factories. The results can be found here:

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)?
msg9851 (view) Author: silvan Date: 2021-01-27.11:23:17
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 for the correct workflow.
msg9848 (view) Author: thomas Date: 2021-01-27.11:13:46
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?
msg9847 (view) Author: thomas Date: 2021-01-27.11:07:20
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.?
msg9845 (view) Author: silvan Date: 2021-01-27.11:02:06
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.
msg9844 (view) Author: silvan Date: 2021-01-27.11:00:13
Ah, I missed Malte's comment. Then we should consider to break the computation of orderings up and distribute them to the right factories.
msg9843 (view) Author: silvan Date: 2021-01-27.10:59:01
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.
msg9842 (view) Author: malte Date: 2021-01-27.10:49:53
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.
msg9841 (view) Author: thomas Date: 2021-01-27.10:48:52
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).
msg9840 (view) Author: thomas Date: 2021-01-27.10:44:48
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).
msg9839 (view) Author: thomas Date: 2021-01-27.09:47:02
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
msg9838 (view) Author: thomas Date: 2021-01-27.09:36:47
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.
msg9836 (view) Author: thomas Date: 2021-01-27.09:26:33
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).
msg9834 (view) Author: thomas Date: 2021-01-27.08:41:52
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).
msg9831 (view) Author: silvan Date: 2021-01-27.08:30:06
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.
msg9825 (view) Author: silvan Date: 2021-01-27.08:05:24
Add link to meta issue.
msg9819 (view) Author: thomas Date: 2021-01-26.21:23:50
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.
msg9808 (view) Author: silvan Date: 2021-01-26.15:09:30
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.
msg9806 (view) Author: thomas Date: 2021-01-26.12:21:21
[This pull request]( 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](

[Commit 69694b4]( 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:

An experiment for a satisficing configuration is an additional indication that nothing changed so far:
msg9793 (view) Author: thomas Date: 2021-01-22.12:06:30
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.
Date User Action Args
2021-02-03 17:08:33thomassetstatus: reviewing -> resolved
2021-02-02 13:00:39maltesetmessages: + msg9976
2021-02-02 08:52:43silvansetmessages: + msg9975
2021-02-02 08:39:34thomassetmessages: + msg9974
2021-02-01 11:05:18silvansetmessages: + msg9968
2021-02-01 11:04:48thomassetmessages: + msg9967
2021-02-01 11:03:26silvansetmessages: + msg9966
2021-01-30 17:43:22maltesetmessages: + msg9960
2021-01-29 17:33:11thomassetmessages: + msg9952
2021-01-29 12:47:57salomesetmessages: + msg9941
2021-01-29 10:35:35thomassetmessages: + msg9937
2021-01-28 19:34:54maltesetmessages: + msg9907
2021-01-28 19:27:20maltesetmessages: + msg9905
2021-01-28 13:26:11clemenssetmessages: + msg9880
2021-01-28 11:56:57silvansetmessages: + msg9875
2021-01-27 18:31:02silvansetmessages: + msg9863
2021-01-27 18:17:25thomassetmessages: + msg9862
2021-01-27 18:12:32thomassetmessages: + msg9861
2021-01-27 18:06:05silvansetmessages: + msg9860
2021-01-27 17:53:58thomassetmessages: + msg9859
2021-01-27 11:23:17silvansetmessages: + msg9851
2021-01-27 11:13:46thomassetmessages: + msg9848
2021-01-27 11:07:20thomassetmessages: + msg9847
2021-01-27 11:02:06silvansetmessages: + msg9845
2021-01-27 11:00:14silvansetmessages: + msg9844
2021-01-27 10:59:01silvansetmessages: + msg9843
2021-01-27 10:49:53maltesetmessages: + msg9842
2021-01-27 10:48:52thomassetmessages: + msg9841
2021-01-27 10:44:48thomassetmessages: + msg9840
2021-01-27 09:52:58thomassetsummary: The LandmarkFactory class 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. -> 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.
2021-01-27 09:52:52thomassettitle: Move exploration object from LandmarkFactory to its subclasses -> Move functionality of the LandmarkFactory class to its subclasses
summary: This is part of meta issue987. -> The LandmarkFactory class 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.
2021-01-27 09:47:02thomassetmessages: + msg9839
2021-01-27 09:36:47thomassetmessages: + msg9838
2021-01-27 09:26:34thomassetmessages: + msg9836
2021-01-27 08:41:53thomassetmessages: + msg9834
2021-01-27 08:30:06silvansetmessages: + msg9831
2021-01-27 08:05:24silvansetmessages: + msg9825
summary: This is part of meta issue987.
2021-01-26 21:23:50thomassetmessages: + msg9819
title: Clean up LandmarkFactory code -> Move exploration object from LandmarkFactory to its subclasses
2021-01-26 15:09:30silvansetstatus: chatting -> reviewing
nosy: + salome, clemens
messages: + msg9808
assignedto: thomas
2021-01-26 12:21:21thomassetmessages: + msg9806
2021-01-25 19:48:10silvansetnosy: + malte, silvan
2021-01-22 12:06:30thomascreate