Title LandmarkFactory as option/plugin instead of LandmarkGraph
Priority feature Status resolved
Superseder Nosy List florian, jendrik, malte, manuel, silvan
Assigned To manuel Keywords
Optional summary

Created on 2015-11-10.10:01:41 by manuel, last changed by florian.

msg5433 (view) Author: florian Date: 2016-06-09.12:00:00
Looks like this is already merged. I'll close the issue. Feel free to reopen, if
I missed anything.
msg5425 (view) Author: silvan Date: 2016-06-05.18:40:49
(And of course updating the option caveats wiki.)
msg5424 (view) Author: silvan Date: 2016-06-05.18:39:45
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.
msg5419 (view) Author: malte Date: 2016-06-02.14:37:00
Results look fine to me.
msg5417 (view) Author: manuel Date: 2016-06-02.11:34:06
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.
msg5402 (view) Author: malte Date: 2016-05-22.12:31:31
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).
msg5401 (view) Author: jendrik Date: 2016-05-22.12:23:09
My bad, you're right. We don't get the same Exploration object.
msg5400 (view) Author: malte Date: 2016-05-22.12:10:56
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.)
msg5398 (view) Author: jendrik Date: 2016-05-22.00:48:45
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.
msg5392 (view) Author: malte Date: 2016-05-21.20:35:00
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."
msg5391 (view) Author: malte Date: 2016-05-21.20:29:25
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?
msg5390 (view) Author: jendrik Date: 2016-05-21.20:26:30
Florian mentioned weak_ptrs. Maybe they can help us for both caching mechanisms 
(landmark graphs and predefined landmark factories).
msg5389 (view) Author: jendrik Date: 2016-05-21.20:24:49
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 
msg5387 (view) Author: malte Date: 2016-05-21.18:10:20
OK, I think the experiments show that we should avoid recomputing the landmark
graph unnecessarily.
msg5384 (view) Author: manuel Date: 2016-05-20.13:42:38
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

The results of the first three configurations look good to me.
msg5373 (view) Author: malte Date: 2016-05-18.18:27:55
I'm done with my comments, too.
msg5372 (view) Author: jendrik Date: 2016-05-16.11:11:54
For easier reference, here are the pull request and the results:
msg4747 (view) Author: manuel Date: 2015-11-10.10:01:41
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.
Date User Action Args
2016-06-09 12:00:00floriansetstatus: reviewing -> resolved
messages: + msg5433
2016-06-05 18:40:49silvansetmessages: + msg5425
2016-06-05 18:39:45silvansetmessages: + msg5424
2016-06-02 14:37:00maltesetmessages: + msg5419
2016-06-02 11:34:06manuelsetmessages: + msg5417
2016-05-22 12:31:31maltesetmessages: + msg5402
2016-05-22 12:23:09jendriksetmessages: + msg5401
2016-05-22 12:10:56maltesetmessages: + msg5400
2016-05-22 00:48:45jendriksetmessages: + msg5398
2016-05-21 20:35:00maltesetmessages: + msg5392
2016-05-21 20:29:25maltesetmessages: + msg5391
2016-05-21 20:26:30jendriksetmessages: + msg5390
2016-05-21 20:24:49jendriksetmessages: + msg5389
2016-05-21 18:10:20maltesetmessages: + msg5387
2016-05-20 13:42:38manuelsetmessages: + msg5384
2016-05-18 18:27:55maltesetmessages: + msg5373
2016-05-16 11:11:54jendriksetstatus: unread -> reviewing
messages: + msg5372
2015-11-10 10:51:33silvansetnosy: + silvan
2015-11-10 10:01:41manuelcreate