Issue999

Title split landmark and landmark node into two classes
Priority wish Status resolved
Superseder Nosy List clemens, jendrik, malte, silvan, thomas
Assigned To Keywords
Optional summary
This is part of meta issue987.

Pull request: https://github.com/aibasel/downward/pull/51

Created on 2021-01-28.15:33:48 by clemens, last changed by thomas.

Summary
This is part of meta issue987.

Pull request: https://github.com/aibasel/downward/pull/51
Messages
msg10349 (view) Author: thomas Date: 2021-07-02.12:48:31
The change log entry has been added in commit 23eef6a86921340e51eed713eef54deca5380123.
msg10348 (view) Author: clemens Date: 2021-07-02.12:21:13
I think we forgot to add an entry to the change notes in CHANGES.md.
msg10346 (view) Author: thomas Date: 2021-07-02.11:20:28
This was merged in commit 4f552c7231d0ce7ed230f72b4796162624559908.

Thanks everyone!
msg10345 (view) Author: malte Date: 2021-07-02.10:27:58
The results look good to me. I also don't see a reason not to merge.
msg10344 (view) Author: thomas Date: 2021-07-02.09:06:05
In v6, we save a copy of the vector of facts that describe a landmark in the Landmark constructor. The results are here: 

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-satisficing-v6-compare.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-optimal-v6-compare.html

As we can see, this has a slightly positive effect on the landmark generation time. Apart from that, the results are pretty much identical to the baseline and I don't see a reason not to merge this.

What do you think?
msg10340 (view) Author: clemens Date: 2021-06-30.08:09:42
Thanks for the reviews Florian and Salomé! We addressed all the suggested changes and implemented those fitting this issue. After doing so, I ran another experiment with more configurations than the last ones:

https://ai.dmi.unibas.ch/_tmp_files/buechner/issue999-optimal-v5-compare.html
https://ai.dmi.unibas.ch/_tmp_files/buechner/issue999-satisficing-v5-compare.html

I did not find any particular reasons against merging this as is. Probably the most concerning thing is the drop in coverage by 2 in optimal/lm_hm. It might have to do with the increased landmark_generation_time in this case. However, I'd rather open a new issue for refactoring the landmark_factory_hm because I believe there's a lot that can be improved there.

Any other thoughts and opinions?
msg10330 (view) Author: thomas Date: 2021-06-28.10:54:14
Here are the results of the experiment:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-optimal-v4-compare.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-satisficing-v4-compare.html
msg10327 (view) Author: thomas Date: 2021-06-25.14:45:21
Regarding Maltes last message: the running experiment includes v1 again and will also report landmark specific information, thanks for pointing this out!
msg10325 (view) Author: thomas Date: 2021-06-25.14:42:38
The title (and scope) of this issue was changed from "implement classes for different types of landmarks" and now only covers a sub issue of the original issue, which has been moved to issue1023.

The current implementation is at a point where landmark and landmark node information have been separated successfully, and the experimental results looked promising (the final experiment is still running, so we need to wait until we can confirm this last part). We therefore decided to split this off of the original issue and review the current changes as the diff is already large enough.

I'll share the final results once they are available, but it wouldn't hurt if someone had time to look at the pull request in the mean time.
msg10321 (view) Author: malte Date: 2021-06-23.13:03:06
Similarly to other landmark issues, I think the experiments should also report things relevant to the landmark code, such as landmark generation time, numbers of landmarks and orderings etc. This can hopefully be copied from experiments from some of the other landmark issues we worked on recently.

No need to create a new report for this experiment, but perhaps in the next experiment we can also include this data for base and v1.

I agree that the runtime data looks good to proceed. Unlike other landmark issues we have had recently, the data looks largely like noise if you look into the per-domain results. Previously we often had small difference that were quite consistent across domains.
msg10320 (view) Author: thomas Date: 2021-06-23.08:34:32
Pull request added to summary
msg10319 (view) Author: thomas Date: 2021-06-23.08:31:02
In a first step, Clemens and I moved parts of the LandmarkNode class to a new Landmark class that contains all the functionality not related to the graph structure over landmarks and replaced occurences of the LandmarkNode class with the new Landmark class where it was *obviously* possible.

We ran a small experiment with one optimal and one satisficing configuration to make sure we detect possible issues early on:

https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-optimal-compare.html
https://ai.dmi.unibas.ch/_tmp_files/tkeller/issue999-satisficing-compare.html

As it seems to be usual with the landmark code, we get a slight improvement in one experiment and slightly worse results in the other. Most importantly, there doesn't seem to be a fundamental issue anywhere, so I suggest we keep going and care about experimental results when we are closer to having a code version we are happy with.

The next steps to get closer to such a version are:
- use Landmark instead of LandmarkNode where possible (also in the non-obvious cases)
- Redesign the way Landmarks and LandmarkNodes are created
- Decide where Landmarks live and how they can be accessed

Note that this is clearly not an exhaustive list to close this issue, it's just the immediate next steps.
msg9883 (view) Author: clemens Date: 2021-01-28.15:33:47
Currently, a LandmarkGraph consists of instances of LandmarkNode. The LandmarkNode class has several members to denot the type of a landmark:
- disjunctive for a disjunction of facts
- conjunctive for a conjunction of facts
- it is referred to as simple if it consists of exactly one fact

In this issue, we'd like to implement an abstract base class for landmarks. The distinction of different types can then be made using different classes that inherit from the base class. Each node in the LandmarkGraph should refer to an instance of any concrete landmark implementation.

 which have members denoting whether a landmark is simple (refers to one fact which must become t
History
Date User Action Args
2021-07-02 12:48:31thomassetmessages: + msg10349
2021-07-02 12:21:13clemenssetmessages: + msg10348
2021-07-02 11:20:28thomassetstatus: chatting -> resolved
messages: + msg10346
2021-07-02 10:27:58maltesetmessages: + msg10345
2021-07-02 09:06:06thomassetmessages: + msg10344
2021-06-30 08:09:42clemenssetmessages: + msg10340
2021-06-28 10:54:15thomassetmessages: + msg10330
2021-06-25 14:45:21thomassetmessages: + msg10327
2021-06-25 14:42:39thomassetmessages: + msg10325
title: implement classes for different types of landmarks -> split landmark and landmark node into two classes
2021-06-23 13:03:07maltesetmessages: + msg10321
2021-06-23 08:34:32thomassetmessages: + msg10320
summary: This is part of meta issue987. -> This is part of meta issue987. Pull request: https://github.com/aibasel/downward/pull/51
2021-06-23 08:31:03thomassetstatus: unread -> chatting
messages: + msg10319
2021-01-28 15:38:10thomassetnosy: + thomas
2021-01-28 15:33:48clemenscreate