goals (see further details in msg9768):
1. The LandmarkGraph should be constant after creation, particularly it should not track the achieved status of landmarks in the currently evaluated state (issue988).
2. The LandmarkFactory should be cleaned up: it provides many methods as well as access to the exploration object which does not belong there (issue990).
3. We want a common LandmarkNode class from which the different types of landmarks (atomic, conjunctive, disjunctive, disjunctive action) inherit (issue999).
---------
4. Check if special treatment for cycles and landmarks that can be achieved simultaneously is necessary.
5. Look at used data structures and determine if there are better options.
6. Avoid storing information at weird places.
7. Check for obsolete code parts.
(Goals 4-7 are more abstract and might require more discussion)


related issues:
cleanup and redesign:
 - issue160: automatically detect whether preferred operators should be computed
 - issue334: separate landmark generation and heuristic
 - issue384: use variables instead of predicate names for disjunctive landmarks
 - issue686: handle task transformations in path-dependent heuristics (currently there is a guard against accidental misuse, see issue845)
 - issue988: make the landmark graph constant after creation (goal 1)
 - issue989: make LandmarkGraph more readable and adapt code style
 - issue990: separate between relaxation-based and other landmark factories; move exploration to relaxation-based ones
 - issue992: make the landmark code consistent with coding conventions
 - issue993: make landmark graph objects be one-shot constructed by landmark (graph) factories
 - issue995: refactor LandmarkFactory options
 - issue996: create landmark factory that makes landmark graph acyclic
 - issue999: split LandmarkNode class into Landmark and LandmarkNode
 - issue1000: fix performance loss introduced in issue988
 - issue1023: implement LandmarkNode as abstract class from which specific implementations for atomic, conjunctive, disjunctive ... landmarks inherit
 - issue1036: make landmark progression sound and deterministic
 - issue1044: remove dead code in landmark exploration
 - issue1045: refactor landmark exploration (depends on 1044)
 - issue1050: handle landmarks from derived variables in a principled way
 - issue1070: clean up computation of interesting landmarks for preferred operators
 - issue1071: split LM-count in two: admissible and inadmissible
 - issue1074: filter causal landmarks in a landmark factory
 - issue1075: explore ways to define preferred operators in LM-count
 - issue1089: drop obedient-reasonable ordering type
new features:
 - issue124: distinguish between first and all achievers when computing the lmcount heuristic
 - issue202: explore use of reasonable orders in admissible heuristics
 - issue257: allow overlapping landmarks in the same landmark graph
 - issue994: allow cyclic landmark graphs
 - issue1011: determine unsolvability based on landmark graph
 - issue1025: handle costs at right place and identically for all landmark factories
 - issue1154: implement dead-end detection in landmark progression

bugs:
 - issue247: lm heuristic crashes / wrongly reports dead ends
 - issue383: reasonable orders not always generated
 - issue442: lm_rhw detects wrong orderings
 - issue467: assertion failure with unsolvable tasks
 - issue984: segfault in h^m landmark factory
 - issue937: breaking cycles chokes on unsolvable tasks (related to issue467)
 - issue998: lm_zg does not handle unsolvable tasks properly (related to issue467 and issue998)
 - issue1004: lm_rhw produces wrong greedy-neccessary orders
 - issue1009: compute correct landmark costs in all factories
 - issue1041: failing assertion in ZG factory due to wrongly claiming relaxed reachability
 - issue1072: conjunctive landmarks are ignored when computing preferred operatos
 - issue1087: failing assertion in hm factory due to overlapping conjunctive landmarks

important configurations for experiments testing changes in landmark factories:
satisficing:
- lama-first (alias)
- like lama-first however with pref=True in lm_rhw
- landmark_sum(lm_zg())

optimal:
- --evaluator 'lmcp=landmark_cost_partitioning(lm_exhaust())' --search 'astar(lmcp,lazy_evaluator=lmcp)'
- --evaluator 'lmcp=landmark_cost_partitioning(lm_reasonable_orders_hps(lm_hm(m=2)))' --search 'astar(lmcp,lazy_evaluator=lmcp)'
- --alias seq-opt-bjolp
- like seq-opt-bjolp but with optimal=True in landmark_cost_partitioning (this requires an installed LP solver)

> I didn't create an issue so far. Should I do so or is the change "trivial"
> "enough to not do it and just reference this issue987 or the related issue1036 
> in the release notes?

We use the [issue...] tags so that people can find out more about the history and rationale of changes. If either this issue or issue1036 serves this purpose, no point in creating a separate issue just to have something to point to.

(If it's not yet clear to everyone, bear in mind that the old issue branch names are not really part of our workflow any more. The only way in which the issue affects the final result in the repository is via the [...] tag in the commit message.)

I ran such an experiment, changing the alias seq-opt-bjolp to use reasonable orderings. Looks good to me: https://ai.dmi.unibas.ch/_experiments/buechner/bjolp/data/bjolp-v1-eval/bjolp-v1-bjolp-base-bjolp-v1-compare.html

I didn't create an issue so far. Should I do so or is the change "trivial" enough to not do it and just reference this issue987 or the related issue1036 in the release notes?

Sounds good. One reason we use aliases is to make recommendations. We can add reasonable orderings to the bjolp alias if we think they're good to use. In this case it would be good to run a little experiment that we can refer to in the release notes, showing what the performance benefit of this is.

After resolving issue1036 it's possible to use reasonable orderings with optimal landmark configurations. Therefore, I suggest to adapt our standard test set of configurations for landmark issues. How about adding reasonable orderings to the optimal lm_hm(m=2) factory? I've adapted the summary accordingly, but we can also make more adaptions. I would generally recommend using reasonable orderings, but our default configuration (i.e., the seq-opt-bjolp alias) doesn't use them.

Add issue1089.

Update heuristic names due to splitting lmcount into landmark_sum and landmark_cost_partitioning.

Add issue1087.

Add issue1074 and issue1075.

Add issue1072.

Add issue1071.

Add issue1036.

add issue1070

I removed the *reasonable_orders* option from the items in the list of important  configurations in the summary.

I've added issue1050 to the list under "cleanup and redesign". This is a bit arbitrary, I think it would also fit under "new features" and possibly "bugs".

I don't know where to track this because it's such a small thing, but perhaps we also don't want to lose it: on https://www.fast-downward.org/Doc/LandmarkFactory "conditional_effects" should be "conditional effects" in all cases. If we think this is too small of a change for one issue, these requirements can be revamped more generally. The "don't know" cases should be replaced by "do know", the missing entry for lm_hm should be added, and we should also cover derived predicates.

Add issue1041, issue1044, and issue1045.

add issue1023

add issue1011

I had the same problem as Salome desribes in msg10036 and removed the duplicate optimal configuration from the summary

I must have overlooked that one. Yes, this is exactly the same as BJOLP but with lm_hm(m=2) instead of lm_merged(...).

Sorry, with above I meant --evaluator 'lmc=lmcount(lm_hm(m=2),admissible=true)' --search 'astar(lmc,lazy_evaluator=lmc)'.

I don't think so, Salomé. seq-opt-bjolp is an alias for lm_merged([lm_rhw(), lm_hm(m=1)]), which is definitely not the same as lm_hm(m=2). Or did you not refer to seq-opt-bjolp with "the same as above it"?

Just tried to run some experiments with the recommended configurations for issue995, but my experiment complained that "like seq-opt-bjolp but with lm_hm(m=2) instead of lm_merged(...)" is the same as the configuration above it. Did I miss something or could we remove this?

Fix typo.

Update configs.

Link issue989.

Link issue1000.

Update configurations to test in experiments affecting the landmark factories.

When looking into the experiments for issue990 we detected a bug in the lm_zg factory, where unsolvable tasks are not handled properly. I created a new issue (issue998) and added it to the bug section of the summary here.

Summary: add issue996

Ok. Then I suggest to broaden the title of issue990 again because with the current title, I would consider the issue done.

Silvan, I wasn't aware of the first item on your list, and I don't think it fits issue990. I guess it makes sense to have that one be part of the issue that introduces classes for the different kinds of landmarks (goal 3 in the summary). The other 3 are part of issue990.

Summary: update name of issue993.

Thomas, we also discussed various issues related to "post-processing" of the landmark graph and came to the conclusion that we would like figure out the following:
- which landmark factory generates which types of landmarks
- based on the findings, make options such as "only causal landmarks" only available to factories where it makes sense
- the same applies for approximate_reasonable_orders and reasonable orders, and maybe also making the graph acyclic
- let all individual factories be responsible for post-processing (removing orders, making acyclic etc.)

I thought that we decided to figure these things out in issue990, but I saw that you wanted to treat this one is done. This is fine, but the successor issue993 also doesn't sound like it wanted to tackle these items. Where do you want to tackle the above items? (I saw that there is issue994 for looking into the acyclic graph issue.)

Add to summary:
- issue990: separate between relaxation-based and other landmark factories; move exploration to relaxation-based ones

We discussed several issues with the landmark code today, and we didn't know what to do with the following two:

- the class LandmarkFactory contains a variable no_orders (which is protected once issue990 is merged) that is set by a command line option "no_orders" and a protected function use_orders that returns the negation of no_orders. It should be sufficient to directly access the variable in the subclasses of LandmarkFactory, and I find it confusing that the command line option is a negated concept. I'd propose to change this s.t. there is only a variable "use_orders" and s.t. the command line option is "positive" (i.e., it's called use_orders as well).
- The class LandmarkFactoryMerged exits if it encounters a conjunctive fact landmark. I don't see a conceptual issue with this, but even if there was one, it doesn't make sense to live with the conceptual issue if it is implemented in a landmark factory, but reject it if the same landmark factory is embedded into a LandmarkFactoryMerged.

We should determine if these two can be ignored, if we should add them to the summary or even create issues for them.

Add new issue988 for refactoring the LandmarkGraph class as described in msg9768 point (1.).

We had a meeting yesterday with the aim of creating subtasks for this issue. We decided to start with the following three tasks (these might be further divided into smaller bits, looking closer at the code will tell):

1. The LandmarkGraph should be constant after creation. Currently, landmark nodes have a status that is used to track which landmarks have been achieved, and we'd like to move that functionality somewhere else.
2. The LandmarkFactory should be cleaned up, Currently, the class provides many methods as well as access to the exploration object that do not belong here. Instead, the functionality should be moved to the classes that inherit from LandmarkFactory.
3. The LandmarkNode class is currently a joint implementation of different kind of landmarks: AtomicLMs, ConjunctiveAtomicLMs, DisjunctiveAtomicLMs and DisjunctiveActionLMs. We'd like to replace this with different classes that inherit from a common LandmarkNode class. This task depends on task 1.

We also discussed some further (currently still quite abstract) issues with the code that might be approached in this sprint or might not (I continue with the numbers to make it easier to discuss these):

4. Some LandMarkFactories treat cycles and landmarks that can be achieved at the same time specially. We'd like to check if this special treatment is even necessary.
5. Look at used data structures and determine if there are better options
6. Avoid storing information at weired places
7. Check for obsolete code parts (e.g., the requirement that the task that is passed to a LandmarkFactory must always be the same); it's possible that there are code parts that are dead for all possible / reasonable (?) planner configurations.

To test changes, these two planner configurations use much of the code:
1. Lamafirst (satisficing)
2. Seqoptbjolp (optimal)

- Once we are at the point to run a proper experiment, we should additionally consider these configurations:
3. Lama (non-admissible version)
4. one configuration for every landmark factory not covered by the previous ones

And I found even more open issues concerned with landmarks:

issue247 reports wrongly reported dead ends which might have to do with conditional effects.

And issue442 reports that lm_rhw introduces wrong greedy-necessary orderings.

issue160 addresses that LM-count should automatically detect whether to compute preferred operators or not.

The landmark code is an old and messy part of Fast Downward. During our January 2021 Fast Downward sprint, we intend to clean it up. Below, I summarize problems and related issues in this context.

General:
========
Responsibilities are not intuitive. For example, one core element is the class LandmarkGraph. It not only stores landmarks and orderings (=dependencies) between them, but also information used by the LM-count heuristic. This is also reflected in issue334. There should be a clear distinction between information belonging to the landmarks and information about them required by some other component.

Landmark factories produce such LM-graphs. There are several methods to generate landmarks, which all implement the abstract class LandmarkFactory. However, LandmarkFactory implements several functions which should actually be part of its child-classes. This should be changed accordingly to make the distinction between these landmark generators much cleaner.

Logic:
======
issue202 discusses problems with theory and implementation of approximating reasonable orderings and how they are used to maintain which landmarks are reached/accepted in the LM-count heuristic.

issue257 reports that landmarks are only added to LM-graphs if they do not intersect. It is not clear to me why it is implemented like this. In my honest opinion, this is very random. We should dig deeper to clarify the intention behind this.

issue383 reports odd behaviour when changing another thing which seems very random. The issue was set aside until after the long planned refactoring with the comment "Let's hope its not another 3.5 years." Guess what: this was 4 years ago ;-)

Other:
======
issue124 is concerned with how LAMA deals with landmark costs.

issue384 was an attempt to change the way disjunctive landmarks are derived, but  seems abandoned.

issue686 and issue845 are concerned with using different task representations at the same code level. More details can be found at http://www.fast-downward.org/ForDevelopers/LongTermPlans#Task_Transformations.

Bugs:
=====
After the refactoring, we should also address known bugs of the current code. For example, issue467 and issue937 report failing assertions, and issue984 reports a segfault.