Wiki is updated. 
Follow up in issue153.

> There are a few cosmetic things that remain to be done, but I'll open a
> separate issue for those since this should not keep us the further work on
> the landmark code.

See issue153.

> Erez: can you add documentation for this to the wiki?

In doing so, note that we want to reuse the LM graph in the LAMA example and
note that I think we should rename some of the construction methods (also see
issue153).

Merged and pushed to default. :-)

There are a few cosmetic things that remain to be done, but I'll open a separate
issue for those since this should not keep us the further work on the landmark code.

Erez: can you add documentation for this to the wiki?

I fixed all of the comments.

I tried to templatize parse_object appropriately, but I got some weird compiler 
errors, so instead I went for the easy option. However, I think this is 
something we should do, since we already have a lot of code duplication between 
heuristic predefinition and landmark graph predefinition (the heuristic have the 
special case of the synergy, but we can get rid of that by creating a "synergy" 
predefinition).

We need the !dry_run since in dry run mode, the landmark graph is not created.

Some other comments:
 * LandmarksGraphOptions => LandmarkGraphOptions
   (We should eventually also fix the other names that use "Landmarks" where
   the correct English would be "Landmark", but for now let's at least not
   introduce new ones.)
 * I don't think h_m_landmarks.cc needs to include ../option_parser.h.
 * Don't use inline code in the class definitions unless it's very short or
   time-critical. Inline code adds unnecessary compile dependencies.
   (I'm looking at the LandmarksGraph::LandmarksGraphOptions methods.)

Also, the following code uses unsafe casts. If the object were not a landmark
graph (which I guess cannot happen at the moment, but could happen as soon as we
allow defining other kinds of objects), this will crash:

    void *object = OptionParser::instance()->parse_object(config,
            start + 2, end, dry_run);
    end++;

    LandmarksGraph *lm_graph = (LandmarksGraph *) object;
    if (!dry_run && lm_graph == 0)
        throw ParseError(start);

Old-style casts should generally be avoided. In this case, there is no safe
new-style cast, which should be a warning sign. Possible fixes:

1. replace parse_object with "parse_landmark_graph" for now, and generalize it
later when we refactor the object parsing code.

2. templatize parse_object appropriately.

Also, why the "!dry_run" choice? Isn't this precisely the kind of error we would
like to detect in a dry run, too?

Just looked at the newest revision, 4d15f8744110, and the indentation is messed
up in various places. Please let me know when it's fixed.

Once this is merged, the wiki needs to be updated.
Malte - let me know when you're finished.

I added the landmarks graph to the command line parsing of landmarks count 
heuristic and lm_ff synergy.
I also added an object factory, and an object predefinition option.

So far, so good: I'm reasonably happy with the code now, so I've pushed it to
master. It's still missing some functionality, so I suggest we do another
revision or two on the branch before merging this back into default.

Open questions:
 * Should the admissible heuristics use the disjunctive landmarks? I don't see
   a reason why not, but I don't remember how this is implemented (or for some
   parts I never knew), so maybe there's a reason why it should not. Right now,
   disjunctive landmarks are disabled for the admissible heuristics.

Functionalities of the emil-new branch that are still missing:
* option to control m for the h^m landmarks
* option to disable disjunctive landmarks
* option to disable conjunctive landmarks
* option to disable reasonable orders
* option to disable all orders
* option to disable non-causal landmarks

Erez, you can work on these. Due to the tangled history thing we discussed
earlier, the current state of the code now lives in a new branch called
"issue122". If you pull from master and hg update issue122, you should see it.

OK, let me know when I can work on this.

Better not, I'm in the middle of cleaning up. :-)
Just wanted to know I'm looking at the correct code; there's no need to hurry.

I will add these command line options tomorrow.

Another point. In Erez's integration, the setting for disjunctive LMs is enabled
by default, disabled when using cost partititioning, but ultimately ignored, so
that disjunctive LMs are always used. Erez, what is the correct intent here?


    bool reasonable_orders = true; // option to use/not use reasonable orderings
    bool disjunctive_lms = true; // option to discard/not discard disj.
landmarks before search
    if (admissible) {
        reasonable_orders = false;
        disjunctive_lms = false;
    }
[...]
/*    if (!disjunctive_lms)
        g_lgraph->discard_disjunctive_landmarks();*/

Let me rephrase this. The emil-new branch in SVN has the following options
regarding LM behaviour that are not present in any of the other SVN branches:

[...]
           } else if (*c == 'T') {
                g_disjunctive_landmarks = true;
            } else if (*c == 'U') {
                g_conjunctive_landmarks = true;
            } else if (*c == 'V') {
                g_action_landmarks = true;
            } else if (*c == 'W') {
                lm_type = LandmarksCountHeuristic::hmbased;
                g_hm_landmarks_m = 1;
            } else if (*c == 'n') {
              g_pm_fluent_pruning = false;
            } else if (*c == 'X') {
                g_no_orders = true;
            } else if (*c == 'N') {
                g_only_causal_landmarks = true;
[...]
            } else if (*c == 'Z') {
                g_reasonable_orders = true;

The version I'm trying to integrate (592567104e52) does not have any argument
parsing related to the new options that I could see. It doesn't have any options
that are not already present in the default branch of master.

You wrote that you added command-line parsing for the new format, but I don't
see any code that does that. So are you sure that you added this code? And if
yes, what revision is it in?

I meant that the argument parsing is new to the emil-new branch.
Everything else should be almost identical to emil-new.

On Mon, Nov 1, 2010 at 6:12 PM, Malte Helmert <
downward.issues@googlemail.com> wrote:

>
> Malte Helmert <helmert@informatik.uni-freiburg.de> added the comment:
>
> > What I did was basically to take the landmarks stuff from the
> > emil-new branch, and add command line options according to the
> > new format. So the functionality should be the same as the
> > emil-new branch, except I may not have exposed all options
> > to the command line.
>
> I did not see any new argument-parsing code -- maybe I'm looking at the
> wrong
> version? I remember there was a bit of confusion about the merging process
> when
> we worked on this while you were here, and there are two abandoned branches
> in
> the code. Can you tell me which version exactly I should be looking at?
>
> (Update to that version, confirm it's the right one, then run "hg
> identify".)
>
> _______________________________________________________
> Fast Downward issue tracker <downward.issues@gmail.com>
> <http://issues.fast-downward.org/issue122>
> _______________________________________________________
>

-- 

--------------------------------------------------------------
"Adventure is just bad planning."
    Roald Amundsen
    Norwegian Arctic & Antarctic explorer
    (1872 - 1928)
--------------------------------------------------------------

> What I did was basically to take the landmarks stuff from the
> emil-new branch, and add command line options according to the
> new format. So the functionality should be the same as the
> emil-new branch, except I may not have exposed all options
> to the command line.

I did not see any new argument-parsing code -- maybe I'm looking at the wrong
version? I remember there was a bit of confusion about the merging process when
we worked on this while you were here, and there are two abandoned branches in
the code. Can you tell me which version exactly I should be looking at?

(Update to that version, confirm it's the right one, then run "hg identify".)

What I did was basically to take the landmarks stuff from the emil-new branch, 
and add command line options according to the new format.
So the functionality should be the same as the emil-new branch, except I may not 
have exposed all options to the command line.


I don't mind setting  pm_fluent_pruning = false  - I don't even know what it 
does

I am completely fine with using the on-the-fly action landmark discovery - this 
is one of the main reasons I switched to this branch completely.

2. I'm fine with getting rid of it.

3. I have nothing to add to this.

In fact, after we're done with this, I suggest we create a new issue about 
refactoring the landmarks code (or maybe - rewriting the landmarks code).

Since I wrote that code in the emil-new branch, I can answer two of the
three questions:

2. That method was indeed just written as a temporary test. It infers
conjunctive landmarks from a given set of landmarks (found via backchaining,
for example). It does not nearly find as many conjunctive landmarks as the
Zhu/Givan method, and the landmarks that it does find probably offer little
additional information over the original set of landmarks from which they
are derived. I'd get rid of it.

3. Yes Malte, your understanding is correct. And I agree that it does make
sense to port this change to the trunk. That was actually one of my
long-standing TODOs, which can then be taken off the tracker.

To get us started, here are three questions:

1. In the emil-new branch, the whole cost partitioning stuff is done completely
   differently. For example, it doesn't use the precomputed action LMs at all
   but rather uses some "on-the-fly discovery" to find actions that are the
   sole achievers of a given LM. The way I see it, the emil-new-integration
   branch discards the old implementation of cost partitioning completely in
   favour of the new one. Erez, is that what you want? (Definitely fine with me;
   just want to confirm you're aware of this effect of the patch.)

2. The integration branch contains a new method
   LandmarksGraphNew::make_conjunctive_lms() which looks strange to me. It
   contains all sorts of debugging output (such as 'cout << "bla" << endl')
   and is never called. The only place that could call it is commented out.
   Can I remove this?

3. Current default says:
       enum edge_type {n = 4, gn = 3, r = 1, o_r = 0, ln = 2};
   but the integration branch says:
       enum edge_type {gn = 3, n = 2, r = 1, o_r = 0};
   However, the default code never generates edges of type "n", and all cases
   where the old code generated edges of type "ln" have been changed to edges
   of type "n" in the integration branch. My interpretation of this is that:
   * in default, "ln" stands for natural and "n" for necessary
   * in integration, "n" stands for natural and necessary does not exist
   * the generated orderings that were "ln" before and are now "n" are indeed
     natural ones
   Under these interpretations, the code changes appear to make sense, although
   I still have to check a few places to see if the change of meaning for "n"
   from "necessary" to "natural" is properly reflected everywhere. But first of
   all, am I understanding this correctly?

I've started looking into Erez's patch for this. So far, I've managed to reduce
the diff to "only" 3988 lines, but there should still be scope for further cleanup.

One question (to Erez): which features exactly did you port from that branch,
and which ones did you not port? For example, it looks to me like the various
options for creating different kinds of landmarks are still present in the code,
but they are no longer controllable by the command line. Right? Anything else
that was not ported?

One thing that looks fishy to me is that the code sets "pm_fluent_pruning =
true". As far as I know, this is a buggy configuration, and I'd prefer not to
merge that option at all. Any objections?

Split off from issue20.

The emil-new branch has a bunch of nice features worth migrating:

 * more efficient (especially w.r.t. space) implementation of some aspects of
   the LM heuristic
 * more options for LM construction (e.g. enable/disable reasonable orders,
   enable/disable disjunctive LMs)
 * the conjunctive LMs from the ECAI 2010 paper by Emil, Silvia and me