Part of issue509.

Thanks, everyone.

I merged this but had to do a little adjustment in planner.cc to avoid computing
is_unit_cost in help mode where no task is available. The diff for that commit
is at 
https://bitbucket.org/FlorianPommerening/downward-issues/commits/64f5d9226dde60d98a3d207dca7096712ca538c8

(The method is_unit_cost is deprecated and should be replaced by the task
interface eventually).

Agreed.

The diff between the two versions looks fine to me:
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-v3-opt-issue700-v2-issue700-v3-compare.html
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-h2-total_time-v2-v3.png

I guess that means that we changed something in on of the last merged issues
that makes the impact of this issue a bit stronger. From my point of view the
difference is small enough that we can merge either version and I'd prefer the
one with the class file in the .cc file.

Yes, currently v1 and v2 are based on different revisions on the default branch.
I'll create a new version that has a clean diff to v2 and run an experiment with
just h^2.

This is strange: if all methods are virtual and no implementation is provided in
the header file, there should be no performance difference. Can we get a clean
experiment that only compares this change? The way I understand it, this is not
what we currently have?

Yes, the diff is just moving the class declaration from the .h to the .cc file:

https://bitbucket.org/FlorianPommerening/downward-issues/commits/27f0234ba6552d5757c9c30e5a47e618aa262070

Based on h^2 runtime, my instinct is to rather go with the faster version. Is it
possible to see a code diff between the slower and faster version?

Could you please produce a report that directly compares v1 and v2? Ah, I see 
that v2 contains additional changes from the default branch. Well, never mind. 

I don't know if the impact of moving the class to the .cc file is big enough to 
influence our design. We could just choose the code version that we like 
better. In my opinion encapsulating the class in the .cc file is a good idea 
and clarifies the interface.

I tried profiling this and saw that most of the time was spent creating tuples
of facts. Interestingly, the only method of RootTask that showed up in the
profile (getting the effect of an operator) was faster in the new version.

Looking at the code again, I noticed that we could move the whole class to the
cc file to reduce the size of the interface. I tried this but unfortunately, it
made things worse:

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-v2-opt-issue700-v2-base-issue700-v2-compare.html
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-h2-total_time-v2-base-v2.png

My suggestion is to roll back the last diff and merge the previous version.

I think the experimental results look good. The only configuration where 
total_time visibly increases is astar(hm(2)). However, the increase is never 
greater than 6%. You could profile one of these runs and try to find out where 
the additional runtime comes from, but I'd be fine with merging this in any case.

I don't have time to look into this, but if the others are happy with the
experimental results, I'm happy to see it merged. (The code was already reviewed.)

The repeated experiment of EHC/FF also looks fine:

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-v1-sat2-issue700-base-issue700-v1-compare.html
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-ehc_ff-total_time-base-v1.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-ehc_ff-memory-base-v1.png

The results for ipdb and h^2 are in. The error in slurm.err looks like it
happened while trying to publish the report.
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-v1-opt2-issue700-base-issue700-v1-compare.html

In h^2 the total time noticeably decreases but the decrease is within 5%. 
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-h2-total_time-base-v1.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-h2-memory-base-v1.png

In iPDB there are changes within +-10% but no clear trend one way or the other.
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-ipdb-total_time-base-v1.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-ipdb-memory-base-v1.png

Maybe it is the validator that is running out of memory. The error happens on 5
PSR-large tasks in all four configurations ({lama,lama-first} x {base,v1}). All
run and driver logs for these runs are empty even though sas_plan files exist
and in the slurm log there are 20 lines saying that a memory limit was exceeded
which probably correspond to those tasks. Either way, I don't think this
influences this experiment because the planner behaves the same before and after
the patch.

> The remaining errors all look like either warnings for >1MB logs in lama or
> like the translator running out of memory on very large PSR tasks.

That would be strange. For example, on my machine translating p34 takes 20
seconds and has a peak memory of roughly 600 MiB. It's not very large either; we
could already solve all tasks up to p35 back in 2004.

I updated the report to exclude EHC/FF. The remaining errors all look like
either warnings for >1MB logs in lama or like the translator running out of
memory on very large PSR tasks.

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-v1-sat-issue700-base-issue700-v1-compare.html

I'll run the new experiments when the grid is back online.

I added items 2, 4, and 5 from msg6840 to issue509 and created issue764 for item 3.

I would like to ignore the satisficing experiment until we have a reasonable
number of unexplained errors in the report. I think it's too easy to miss
something when there are hundreds of unexplained errors. (If it's easy to
produce a report that excludes the EHC configuration, I'm happy to look at it.)

Regarding the other experiment, it looks like the things in "unexplained errors"
aren't errors but warnings, and perhaps it would be nice to be able to make this
distinction in the reports. Of course this is unrelated to this issue.

The blind search and LM-Cut results look good.

I'd be interested in experimental results for h^2 and iPDB because these
previously stuck out as making a lot of global state/operator accesses and had
some performance issues related to these.

One the satisficing results are updated with fewer unexplained error, I'd like
to see what happens with lama-first in Airport, Satellite and PSR. This doesn't
require a special report. It's mainly a note to self.

Regarding msg6840:

I think that items 2, 4 and 5 are conceptually part of "the task interface", so
they should be tackled as part of the surrounding meta-issue for this. For me it
would be sufficient at the moment to include them in the TODO list for the
meta-issue. Concrete issues for them can be added now or later.

Item 3 should be addressed at some point, but doesn't look very related to this
meta-issue. I think it is somewhat related to the option handling, so if we have
a meta-issue for that, I'd add it there. But I'd also be OK with ignoring it for
now.

I agree on item 1.

The experiments with satisficing configurations contained an error but results
for lama-first are ok: coverage goes down by 2 tasks (one because of memory, one
because of time) but looking at the plots, I don't see a systematic bias.
Overall, the coverage of LAMA is not affected. The hiking domain sticks out of
the memory plot, I don't know why.

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-v1-sat-issue700-base-issue700-v1-compare.html
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-lama-first-total_time-base-v1.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-lama-first-memory-base-v1.png

I'll rerun EHC/FF. Should I add any other configurations?

Experiments on the optimal suite with A*/LM-cut and blind search look fine to
me: there is no change in cost or expansions, LM-cut shows a slight trend to
faster runtimes and better memory usage. For blind search the variance of
runtimes a bit higher than usual but most changes are within +/- ~10% and I
don't see a trend in one direction or the other. Memory usage doesn't change
much, except for getting better in very small instances.

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-v1-opt-issue700-base-issue700-v1-compare.html

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-lmcut-total_time-base-v1.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-blind-total_time-base-v1.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-lmcut-memory-base-v1.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue700-blind-memory-base-v1.png

Thanks for the review. Experiments are on their way.

I completely agree with handling the other global variables later.
Coincidentally, the example you picked (g_variable_domain) is the last
occurrence of that variable. It was already unused and I removed it now.

Out of interest, I had a look at the other g_* variables that are left:

1) g_log and g_timer should probably stay.

2) g_root_task could be replaced by a local variable eventually.

3) g_plan_filename, g_num_previously_generated_plans, and
g_is_part_of_anytime_portfolio could be collected in a class that handles the
creation and storage of plans eventually. 

4) g_axiom_evaluator, g_state_packer, g_successor_generator,
g_initial_state_data should all be accessed through the task interface eventually.

5) g_use_metric is currently only used for debug output in dump_everything(). We
could remove it in this issue or enable access to it through the task interface
later.

Should I create issues for any of these?

PS: To emphasize, if we close this issue, it doesn't mean this is all already in
the shape that we would ideally want to have at the end. But I think any further
changes should rather be part of separate issues that can be individually vetted
(and experimented with).

Code looks good to me. There are still some task-related global variables in
globals.cc that should eventually go (like g_variable_domain), and if we don't
have an issue for that yet, we should probably create one (and mention it in the
task class meta-issue, issue509). But I don't see this as part of this issue.

We need experiments for this now, exercising a variety of configurations (at
least: blind search and a decent collection of optimal configs; some satisficing
configs including some good ones).

This is now waiting for a review by Malte in the review queue.

I'm done with this round of comments on bitbucket.

With issue748 merged, this looks easier now. I tried to prepare a pull request
for this:
https://bitbucket.org/FlorianPommerening/downward-issues/pull-requests/30

There are still a lot of rough corners but I suggest we don't try to fix all of
them in this issue. The patch is already kind of big, so for example, i wouldn't
want to start changing the successor generator or axiom evaluator at the same time.

We may be able to tackle this earlier by finishing issue747 and issue748.
Issue724 is not really necessary for this.

Issue726 is merged now but the code still uses g_operators (for g and real_g).
To get rid of g_operators completely, we also have to wait for issue724, which
will change the way path-dependent heuristics work, so evaluators can keep track
of the g values and we no longer need to store them in the search node.

Switching get_successor_state to OperatorProxy is issue726.

I updated the pull request with the new code. There are still two global
variables that make the code for this issue awkward: g_operators and
g_initial_state_data. The latter has a circular dependency with this issue but
the workaround for this issue wouldn't be so bad, I think.

We already have an issue in the pipeline (issue725) that will start to simplify
things for g_operators. In the next offline meeting we also want to discuss how
to deal with "g" and "real_g", another place that relies on g_operators. If we
also switch get_successor_state to OperatorProxy then we can remove g_operators
and this issue will be a much cleaner patch. So I suggest to wait until then.

Issue629 and issue688 are now merged.

From some offline discussion: For now, we want to simplify things by
implementing the class we called ExplicitTask so far as RootTask instead of
deriving ExplicitTask from DelegatingTask and then RootTask from ExplicitTask.
This will then only work for the root task, not for TNF tasks as we originally
planned in issue576. Once it is merged, we can think about how to best implement
TNF task.

I started working on this and put up a pull request, but I suggest we wait with
this until at least issue629 and and issue688 are merged.

https://bitbucket.org/FlorianPommerening/downward-issues/pull-requests/30

Split off from issue576.

We need ExplicitTask to store the data for the RootTask interface.