Forgot to set this to resolved, it seems.

Merged. Thanks!

No objections from my side.

Given this, I'd be willing to merge this. Any objections?

Thanks! The data is a bit hard to interpret, but I think it supports the
hypothesis that we previously behaved more like greedy BFS than weighted A* and
have now fixed this. The improved quality is one hint, but there is also
something else that is interesting. If we compare the three configurations:

- A: greedy, old
- B: WA* with weight 3, old
- C: WA* with weight 3, new

then if we didn't have a bug we'd expect A to be quite dissimilar to B and C,
while B and C should be similar. I compared the coverage of these configs in all
domains. There are 57 domains. Of these, there are 22 in which all three have
the same coverage and 15 in which all three have different coverage. Comparing
the remaining 20:

- There are 16 where A and B have the same coverage, which differs from C.
- There are 3 where B and C have the same coverage, which differs from A.
- There is 1 where A and C have the same coverage, which differs from B.

I think this is strong evidence that our weighted A* behaved quite a bit like
greedy before.

Here are the results:
http://ai.cs.unibas.ch/_tmp_files/seipp/issue558-v1-ext-issue558-base-issue558-v1-compare.html

Regarding 1) Yes, I noticed that. I fixed it in lab.

Regarding 2) Experiment is running :)

OK, two separate points:

1) The unexplained errors with error "unexplained-search-parser-crash". These
are all for unsolvable tasks with initial state value infinite. Perhaps lab has
problems parsing the new output for states with initial state heuristic values
that are infinite?

2) The changed performance. From my reading of the code, it's quite possible
that the previous version of the code more or less ignored the g values after
issue77 was merged, since "current_eval_context" ignored g and the successor
contexts were made to reuse the cache.

Regarding 1), can you look into this? (no hurry)

Regarding 2), can you run an additional experiment that only considers the
FF-based config (for simplicity), but adds two additional ones: one lazy_wastar
with weight 1000, and one config using lazy_greedy instead of lazy_wastar (but
still with preferred=h, and perhaps also with reopen_closed=true to make it
similar as possible)?

Here are the experiment results:
http://ai.cs.unibas.ch/_tmp_files/seipp/issue558-v1-issue558-base-issue558-v1-compare.html

It seems there is something going wrong indeed. I don't have time now to look into this. The 
experiment is located at /infai/seipp/projects/downward/experiments/issue558/data/issue558-v1.

I pushed a fix, but it is not merged yet. There were two independent bugs:

1) In the computation of the "reopen" flag, we could trigger an assertion in
debug mode. This was harmless in release mode. Basically, we computed
uninitialized values in some cases which the debug assertions guard against, but
these couldn't affect the overall computation anyway.

2) The evaluation contexts for the next node to be expanded did not include g
value information, but the heuristic computation could request it indirectly in
some places.

I think for our standard configurations, #2 wouldn't affect the result either,
but I'd rather be sure about that. Jendrik, can you run a experiment for this?
We only need to test lazy_wastar configurations, say the ones from our standard
test set if there are any. It may be a good idea to run both in debug and in
release mode.

I have pushed a branch with the fix to the master repository, but it's not
merged yet.

It's independent of the weight and of the heuristic; the fact that g() is used
in the evaluator is the important bit. This is a simple configuration for
testing it:

    ./downward-debug --search "lazy_wastar(blind(), w=1)"

An obvious problem is that in this line in lazy_search.cc:

bool reopen = reopen_closed_nodes && (current_g < node.get_g()) &&
!node.is_dead_end() && !node.is_new();

we may try to obtain the g value of an uninitialized node, which could be fixed
by moving the is_new() test earlier in the order. (If the node is not new, it
will have a well-defined g value.)

But it seems fixing the above line is not enough. Later on, it seems that the
code wants to test if the node is a dead end before opening it; but before the
node is opened, its g value is invalid. I think perhaps the opening/reopening
stuff and the g values are too tightly coupled at the moment. It would help if
we always made g values of nodes valid as soon as possible.

The following configuration violates an assertion:

./fast-downward.py --debug ../benchmarks/miconic/s1-0.pddl --heuristic "h=ff()" --search "lazy_wastar(h,w=3,preferred=h)"
...
downward-debug: search_space.cc:39: int SearchNode::get_g() const: Assertion `info.g >= 0' failed.

The problem is solved without the --debug option. The assertion is violated for any w in {0,1,2,3}.