Great, thanks!

I merged the code and announced the change on the public mailing list.

Yes, I forgot that we already have an issue for that: 
https://bitbucket.org/jendrikseipp/lab/issues/39/check-slurm-logs

> If you'd like to discuss this further, we should maybe take the discussion to
> the Lab issue tracker or offline.

Sounds good. Can you point me to the lab tracker issue?

Regarding slurm.err: Lab now includes the slurm error log in reports. It tries 
to skip the "memory cg" lines. Since the output is often mangled up, this 
doesn't filter all occurences of these lines. If the slurm.err file is missing 
during the creation of the report, but it was non-empty during fetching, the 
report shows a warning. Here is the updated report: 
http://ai.cs.unibas.ch/_tmp_files/seipp/issue743-v3.html
If you'd like to discuss this further, we should maybe take the discussion to 
the Lab issue tracker or offline.

Looks excellent indeed! 35 more solved tasks, spread over 12 domains. It is
noticeable that all of these are transportation domains. I suppose this is
because they commonly have this fork-shaped causal graph pattern where a
non-goal variable (vehicle) supports two different goal-variables (packages).
Definitely worth informing Patrik once it's merged.

Jendrik, I think it's important to explain in the code comments *why* we are
defining the neighbourhood in this way since it's different from what is said in
the paper and has escaped us for a long time. Can we work on this together later
today? I'm flexible regarding time apart from the lecture (14:00-16:00).

BTW, I'm not so happy about the way the unexplained errors are reported because
we cannot tell *what* the output to slurm.err was. I don't think we should get
into a habit of always ignoring all output to slurm.err. Let's discuss this
later, too.

The results are in and they look very good:

http://ai.cs.unibas.ch/_tmp_files/seipp/issue743-v3-issue743-v2-issue743-v3-compare.html

v2: Consider co-effect goal variables
v3: Additionally consider goal variables for which the pattern is relevant

Your argument makes sense, I think. I'll implement this and run an experiment.

On further reflection, the case where iPDB misses something shouldn't be so rare
after all. Consider a problem with three variables u, v, w, where v and w are
the goal variables, there are no eff--eff arcs, and the pre->eff arcs are u->v
and u->w. Then the pattern {u, v, w} is interesting, but iPDB will never
consider it even though it considers {u, v} and {u, w}. The modified algorithm
would consider it. I guess fork patterns of this kind are not that unusual.

(Of course, the modified algorithm still doesn't help if the path from the
forking variable to the goal consists of two steps, like in a problem with
variable u, v, w, v', w', where v' and w' are the goal variables, there are no
eff--eff arcs, and the pre->eff arcs are u->v, v->v', u->w and w->w'. That's an
inherent problem if you want to grow one variable at a time and make progress
with every single addition. As I said earlier today, I think it would be
interesting to look into other pattern generation methods in the future. I think
that there is a lot that can be done there.)

PS: If this indeed an omission in the iPDB paper, we should probably let Patrik
know and add an errata to our home page. Can you remind me of this if and once
we've decided what the correct behaviour is?

Coincidentally, we'll discuss iPDB in the planning course tomorrow. Today I
checked the slides that I prepared last year for this, and it seems that there
is another case that (I think) we currently don't consider and which was also
forgotten in the original iPDB paper. (Or at least that's what my LaTeX comments
from last year say. ;-))

Correct me if I'm wrong:

1. We start with singleton goal patterns.

2. In our previous implementation, to extend a pattern P we only considered
adding variables v such that v --> u with u in P is a pre-->eff arc in the
causal graph.

3. In the code implemented for this issue so far, we additionally consider v if
v -- u with u in P is an eff--eff edge in the causal graph and v is a goal
variable. This is also how iPDB is described in the original paper, except that
it doesn't include the test that v is a goal variable. But this doesn't affect
the "completeness" of the neighbourhood: it mainly means that it might consider
some unnecessary neighbours that cannot lead to improved heuristic.

Now, according to the lecture slides, we should also consider another case:

4. If u --> v where u in P, v is a goal variable, and u --> v is pre->eff arc,
then v should be a candidate.

This case is easy to miss because in this case the newly added variable v is not
relevant to the pattern so far. However, *the pattern so far is relevant to v*,
and this is also good enough for v to affect the heuristic value.

Does this make sense?

The various rules can be combined and summarized as "consider pre->eff
predecessors of all variables in the pattern, and consider *any* neighbour
(pre->eff, eff->pre, eff--eff) that is a goal variable".

If this is correct and I summarized it correctly, I think it makes sense to also
try out this version. I don't expect a huge difference because it's not so
common to have eff--eff arcs (which we already test) without pre->eff arcs in
both directions becaues most effects come with a precondition. But I think it's
still the correct way to do things. Even if there is no benefit in making this
change, I'd be interested to see if there is ever a case in the benchmarks where
this change leads to a different neighbourhood than the old implementation. I
don't know how easy this is to test -- one option is to test it explicitly by
temporarily having both neighbourhoods in the code and comparing them.

Looks good to me and ready to merge. I made one final comment to the last commit.

I updated the code.

Do you want to update the code in this direction, or does anyone object?

I agree that this could be seen as a bugfix, so keeping the option around doesn't 
seem to add too much value.

> It seems Patrik's original implementation used co-effect goal variables

Yes, our previous restriction was unintentional. (Hence the code comment.) I
think this dates back to when we used the "legacy causal graph", where we did
not distinguish properly between pre->eff arcs and eff<->eff arcs. Some parts
assumed that eff<->eff arcs were included while others assumed they were not,
and the assumptions didn't always agree with reality. When we converted this to
the new causal graph representation, we didn't reconsider this aspect.

I think this is ready to merge, except that I think we can still improve the
option name and documentation. The name and description are currently quite
technical.

To elaborate on this: the iPDB idea is to consider *all* neighbouring variables
in principle. The reason why we restrict to a subset is that adding disconnected
variables can never lead to an improved heuristic value, so they would never be
selected. So not looking at all variables is a pure speed optimization, not
something that affects the behaviour (apart from how it affects the timeouts and
memory limits to consider additional useless patterns).

With the new option set to true, we really consider all viable options. That we
didn't do this before was at least an oversight and probably a bug. Thinking
about this some more, I wonder if we should really keep the old behaviour around
as an option, or simply change the behaviour without introducing an option. What
do you think? If we do keep the old behaviour around, I think we should come up
with a clearer option name and elaborate the documentation.

We should definitely announce the change. 

It seems Patrik's original implementation used co-effect goal variables:

"Adding a variable V to pattern P will only improve over the value of h^P if the 
variable directly influences any of those in the pattern. The causal graph is a 
common tool for analysing influences among variables in a planning problem: a 
variable V' can influence a variable V if some action that changes V has a 
precondition or effect on V'. [...] Variables that can not influence a pattern 
are not considered for extending it."

Together with the empirical advantage, I think it makes sense to use co-effect 
goal variables by default.

Excellent! One last concern: this is a behaviour change, not just an
optimization of the implementation, and it may be unexpected for people who want
to "compare to iPDB". If we merge this, should we send an email to the Google
groups list to let people know? Or at least to downward-dev?

Here are the results for revision v2:
http://ai.cs.unibas.ch/_tmp_files/seipp/issue743-v2.html

And here is the comparison of v2 vs. the base revision (both ignoring co-effect 
goal variables):
http://ai.cs.unibas.ch/_tmp_files/seipp/issue743-v2-vs-base.html

(Both experiments had the cgroups slurm error, but it didn't affect any 
results.)

I agree, the results look quite good. We have to be a bit careful in
interpreting the coverage increase because I think one of the parcprinter
benchmarks is a subset of the other. So we're really only improving in one
domain (by 4 tasks) and getting worse in one other (by 1 task). Time also
becomes worse overall, but evaluations get better. It's quite possible that this
is because we now spend more time in the hill-climbing stage where we previously
would have to give up, which is not necessarily bad.

I commented on the code in Bitbucket, and I think it would be nice to do a
revision before merging. Perhaps that can even help a bit with the increase in
runtime, although I doubt that the modified function is the main bottleneck.

The experiment is finished and I think the results look good:

http://ai.cs.unibas.ch/_tmp_files/seipp/issue743-v1.html

The code is here: https://bitbucket.org/jendrikseipp/downward/pull-requests/75 .

Just in case this wasn't the plan anyway: it would be nice if this could be
controlled by an option.

Pommerening et al. (IJCAI 2013) define interesting patterns as follows 
(Definition 6):

A pattern P is interesting if
1. the subgraph of the causal graph induced by P is weakly connected, and
2. the subgraph of the causal graph induced by P contains a directed path via 
precondition arcs from each node to some goal variable node.

The hill climbing code currently only adds variables to a pattern that are 
connected to one of the variables in the pattern via a precondition arc. We 
would like to test whether it is beneficial to also add *goal* variables that 
are connected to one of the variables in the pattern via a *co-effect* arc.