Marking this as resolved. What we originally wanted to do in this issue is done now, and our other plans for improving reasoning about orderings etc. are covered by issue987, the landmark meta-issue. Different landmark instances as an explicit concept are not yet on the agenda, but I think it's too early to open an issue for this. Someone would need to do some research on this first. :-)

I'm fine with doing it in stages, and the Skype chat can wait (although if the 
semester ends in late June, we might as well discuss it during ICAPS).

One point about your suggested solution with allowing cyclic orderings:
suppose X(1) -> Y -> X(2)  (where X(1) and X(2) are the two instances of X), but 
we also know that X -> Z. If we allow cyclic orderings, we would get:
Z <- X <-> Y  which seems to indicate that Y can be achieved before X, which is 
not the case. However X(1) -> Z
                      X(1) -> Y -> X(2)  captures this information.
We can discuss this during the Skype chat (or in Brazil).

> Actually, if the code is hard to follow now, imagine what it would look like
> after two major refactorings...

Much much cleaner! That's the only point of refactoring. :-)


A complete reimplementation is an option that was on the table (Silvan and I
discussed this a bit last week), but we agreed that given the size of the
codebase, an incremental strategy towards cleaner code is likely to work better.
In my experience (and the people I've read agree), the correct strategy is always:

1. Make it clean.
2. *Then* extend the functionality.

Mixing the two into one step is often a recipe for disaster. :-)

I agree that we should take our future use cases in mind while doing the
refactoring, though. But I think we're already doing that.

(For what it's worth, to support the use case you mention, we don't actually
need separate landmark instances. Rather, we need to get rid of the
ill-conceived notion that cyclic orderings are a problem, and get rid of the
cycle breaking, then see if and how we need to change the code to adapt for that.)

Regarding a skype meeting: it would be good to have a more general discussion
about the next steps at some time. I'd suggest waiting until the end of the
Basel semester though; otherwise it's hard for Gabi and me to find time.

Well, for now, I am going to move the cost calculation code. I think this will
make the code structure more clear, but it is certainly not a major refactoring
but a rather small one, thus the work should not be wasted even if we decide to
change the design afterwards.

Maybe we can first discuss this with Malte before I actually start implementing
a Landmark-Class.

Actually, if the code is hard to follow now, imagine what it would look like 
after two major refactorings...
I think this is an even better reason to think about what we want to do, design 
the classes and interfaces from scratch, and implement this cleanly.

Having said that - since you're doing the work, you get to call the shots. If 
there's anything I can do to help, let me know.

I think this would be a next step after the original topic of this issue has
been resolved, as this needs to be done anyway. As soon as the interface between
LandmarkGraph and LandmarkCountHeueristic is more clear, we can certainly
discuss about further enhancements/generalizations. I already discussed the idea
of a Landmark-Class with Malte and we agree that this is also a necessary
improvement.

I think that extending a Landmark-Class further to support your idea of landmark
instances could then be implemented even more confortably, but doing both at the
same time does not seem to be a great idea to me. Already now, I have some huge
problems understanding what exactly is going on in which part of the code and I
really don't want to mess this up even more.

I recently thought about this, and I have some ideas which might require a 
complete redesign. 

Consider the following scenario: Y is a landmark, X is another landmark which is 
greedy necessary before Y, every achiever of Y deletes X, and X is a top-level 
goal. Then we know that any plan must achieve X, then Y, then X again (X -> Y -> 
X). Currently, this is something that neither our code nor our definitions 
support, although it could certainly be useful. In fact, this is a 
generalization of the work we started on counting how many times each landmark 
needs to be achieved.

This leads me to suggest that we work with "landmark instances" rather than 
landmarks. In the example above, the first X and the second X would be different 
landmark instances (of the same landmark). 

I also think we might want to have a general class of Landmark, and then we 
would have some sub-classes: SimpleFactLandmark, DisjunctiveFactLandmark, 
ConjunctiveFactLandmark, SimpleActionLandmark, ...
with an interface which allows to check if some transition (or state) satisfies 
the landmark. Then our code would support arbitrary formula landmarks (at least 
in theory).

Should we maybe have a Skype chat and discuss this?

I opened this new issue for the next step of landmarks code refactoring.

The goal is to make a strict distinction between the landmarks generation and
the lamarks heuristic. Among other "problems", there are some confusions
concerning computation of landmark costs, e.g. some of those are made during the
landmarks generation rather than by the heuristic during search.