Looks like this is already merged. I'll close the issue. Feel free to reopen, if
I missed anything.

(And of course updating the option caveats wiki.)

I had another look at the hpull request, and found no more things to comment on.

After the open comments are addressed, this can be merged.

Results look fine to me.

Here are the results of comparing two versions of FD. The new version gets a
specification of LandmarkFactory instead of LandmarkGraph from the command line.

In difference to the last experiment (msg5384), the new version of FD in this
experiment caches a LandmarkGraph instead of recomputing it for each heuristic.
This change becomes visible in configurations issue592-v*-lm_hm_max.

http://ai.cs.unibas.ch/_tmp_files/heusner/issue592-v4-lama-opt-issue592-base-issue592-v4-compare.html
http://ai.cs.unibas.ch/_tmp_files/heusner/issue592-v4-lama-sat-issue592-base-issue592-v4-compare.html

Suggestion for now: cache a single object (not keyed on exploration), document
the issue, and leave a proper fix for later. Right now there are other data flow
issues with this code, such as the fact that the passed-in exploration can
potentially come from a heuristic that uses an adapted task (once lm_count uses
that), while the factories themselves always work with the global task. This can
not work correctly.

We know the landmark code is a tangled mess, so I guess it's not too surprising
that some things will be broken while we're in the middle of untangling. I
suggest we also collect a set of landmark test configurations for future
landmark issues and include the ones used for this issue in the set (including
the one with the two h^m heuristics).

My bad, you're right. We don't get the same Exploration object.

Looking at the code again, I agree that the exploration object is used generally
for things like discarding noncausal landmarks.

(BTW, our code for discarding noncausal landmarks sounds like a nonfeature that
would best be removed. Since this analysis is always based on a standard delete
relaxation, it really messes up the responsibilities of the base class in the
sense of mixing aspects of the delete-relaxed landmarks in with all the other
landmarks, and I don't think it serves a useful purpose. I suggest removing this
option in a separate issue. In my opinion, all uses of "Exploration" in the base
class are suspect.)

But even if Exploration is used, this doesn't meant that the suggested caching
strategy would have the desired effect of

 (A) recomputing when necessary
 (B) not recomputing when not necessary

In the example where performance suffers severely because we compute the h^m(2)
landmarks twice, I think we would still recompute because I don't think we would
receive the same exploration object. (I haven't followed the data flow in
detail, but I don't see how we could end up with the same exploration object.)

Concerning the Exploration object: I don't fully grasp what this object does. 
From looking at the code, I think passing a different exploration can produce a 
different landmark graph. The passed Exploration object is used in more than one 
factory subclass and even in the base factory class.

Weak pointers introduce the opposite problem: we might prefer to keep an object
alive to be able to reuse it later, but the weak pointer does not hang on to it
and we lose the cached object.

As I wrote on bitbucket: "it's probably a good idea to think about this issue
[managing object lifetimes] more globally at some other time rather than right
now in the context of this issue."

This seems a bit odd to me. Why is this keyed on the exploration? What does this
object represent?

I thought only one of the factories (lm_rhw) actually uses an exploration
object, although I didn't look at the details again. Also, this suggests that we
can reuse the same landmark graph iff we're using the same exploration object.
Why is it the case that it's correct to use the same landmark graph if we have
the same exploration object? Why is it wrong to reuse if we have a different
exploation object? In the example where we don't get reuse, would this change
fix it?

Florian mentioned weak_ptrs. Maybe they can help us for both caching mechanisms 
(landmark graphs and predefined landmark factories).

OK, I think this means that we'll have to cache the resulting landmark graph 
inside the factory. Or do you have something else in mind? We discussed this a 
bit already on Friday and came up with the following implementation strategy:

The factories have an unordered_map<Exploration *, shared_ptr<LandmarkGraph>> in 
which the graphs are cached. Later, we'll have to change this to 
unordered_map<pair<AbstractTask *, Exploration *>, shared_ptr<LandmarkGraph>>. 
Florian remembered that you once said to be careful that the factory isn't the 
only one keeping an object alive. Even though the factory could in principle be 
deleted after we have obtained all graphs, this doesn't happen in our case, since 
it will be kept alive by the predefinition store. Not sure what to do about that 
though.

OK, I think the experiments show that we should avoid recomputing the landmark
graph unnecessarily.

Here are the results for landmark heuristic configurations:

* Zhu/Givan
* RPG exhaustive
* h^m with m = 2
* max(h^m(admissible), h^m(non-admissible)) with m = 2

The last configuration is for evaluating the time difference between computing a
landmark graph for each h_m (in new revision) instead of one landmark graph for
both h_m (in base revision). Although total_time increases as expected, coverage
is considerably reduced only for two of 46 domains, namely for freecell and
tidybot-opt11-strips.

The results of the first three configurations look good to me.

http://ai.cs.unibas.ch/_tmp_files/heusner/issue592-v3-lama-opt2-issue592-base-issue592-v3-compare.html

I'm done with my comments, too.

For easier reference, here are the pull request and the results:

https://bitbucket.org/manuel_h/fd-issues/pull-requests/1

http://ai.cs.unibas.ch/_tmp_files/heusner/issue592-v3-lama-sat-issue592-base-issue592-v3-compare.html 
http://ai.cs.unibas.ch/_tmp_files/heusner/issue592-v3-lama-opt-issue592-base-issue592-v3-compare.html

Set LandmarkFactory as an option/plugin instead of LandmarkGraph.
It enables LandmarkCount heuristic to pass its task to an instance of
LandmarkFactory for the computation of a LandmarkGraph.
This issue will be done in respect to issue551.