Part of issue509

Great! Merged and pushed. Thanks for working on this in your free time.

Indeed, looks good to merge!

The results on the newest version look much better. There are no more huge 
difference in the total time and memory is also almost not affected.

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue660-v2-issue660-v2-base-issue660-
v2-compare.html

I think this could be merged now.

In some cases, the author of the pull request/commit is not listed under participants 
(probably if they did not yet comment on it). But I think the author is always 
subscribed to all changes automatically. At least for pull requests that seems 
reasonable. I also got notifications for your comments on commits but I'm not sure if 
that was because of some repository setting, because I was the repo owner, or because I 
was the author of the commit.

OK, your version does indeed look odd. Here is my version after I made a dummy 
comment: http://ai.cs.unibas.ch/_tmp_files/seipp/Auswahl_007.png

> You can see who'll get notified about comments by hovering over the
> avatars in the top right corner of changeset pages.

Not sure that works correctly for me. At the very top right corner, I see a
broken image; hovering over it only mentions me, but that may be some
login-related feature. There's also a "Participants" icon close to the top right
corner (at least "Participants" shows up when I hover over the picture of what
looks like two game pieces), but I also only see a broken link there, and
hovering over that doesn't mention a set of watchers. I attached an image. Does
it look different for you? I tried reloading etc. This is from

https://bitbucket.org/FlorianPommerening/downward-issues/commits/522d37628519dd343a65d706727d41603bcf5316

An addition improvement is that we now only access one vector (default_values)
where we used to access two (g_axiom_layers and g_default_axiom_values), which
might help make the code more cache-efficient. In any case, nice to see an
improvement, although of course we don't have data for the grid yet.

Thanks. I did another minor change and added an experiment for the new version. Ahh, 
and yes, the task can now be removed.

I think the speedup compared to default might be because of a range-based loop. If 
this method is so time-critical in this configuration, then changing the loop might 
have made a difference here. Also, not much else has changed in the evaluate method.

Pushed. It looks like there is no longer a need to keep a reference to the task
interface around as an instance variable, as it is only used in the constructor.
So I guess it would make sense to get rid of the instance variable?

At least on my machine, that has done the trick. :-)
It's now even faster than on default.

Comparing total time on PSR-Large (previous issue660 head vs. default head vs.
new issue660 head):

#7: 217.55s vs. 184.91s vs. 177.81s
#6: 21.33s vs. 17.52s vs. 16.68s
#5: 4.33s vs. 3.59s vs. 3.37s
#4: 0.527s vs. 0.456s vs. 0.424s

I'll clean up the patch and push it to your repository to have a look. If it
looks like it may sense, perhaps we should rerun the experiments on the
axiom-using domains to see if it also helps on the grid?

As I wrote, I'm already working on that. Experiments running.

> So we could try to avoid using the task interface as much as possible in
> AxiomEvaluator::evaluate.

The only access to the task interface during evaluation is to determine default values 
and whether a variable is derived or not. I'll try if duplicating this information in 
the  axiom evaluator makes any change.

I looked a bit more at the code; the comparison of the head of issue660 and of
issue660-base have lots of unrelated changes, and it makes more sense to compare
the head of issue660 to the head of default instead. (At least as of this
writing, everything has been merged.) But this doesn't change the numbers of the
baseline substantially, so the differences are indeed due to the changes in the
issue branch.

Also, with blind search, PSR-Large tasks of different sizes seem to behave quite
similarly, comparing total time on issue660 head vs. default head:

#7: 217.55s vs. 184.91s
#6: 21.33s vs. 17.52s
#5: 4.33s vs. 3.59s
#4: 0.527s vs. 0.456s

I'll have a look if there is an easy way to recover some of the lost performance.

Regarding how to speed it up: using the task interface is expensive due to the
indirections which require virtual function class and prevent inlining. In other
contexts, such as relaxation heuristics, I think our general approach is to use
the task interface during setup, but as much as possible avoid using it in the
inner loops of the main computations.

So we could try to avoid using the task interface as much as possible in
AxiomEvaluator::evaluate. I'll have a look and will send an update.

You'll probably see a larger difference when using configurations using less
expensive heuristics. Using '--search "astar(blind())"' on PSR-Large #7, I get
217.55s for the tip of issue660 and 185.099s for issue660-base (on a computer
with negligible load, not running planner processes in parallel).

I tried looking into the speed difference for psr-large task p31. Running the task 
with low background CPU usage I got 71s on the default branch and 77s on the issue 
branch. I repeated this and got 72s and 74s. Running processes for both branches at 
the same time, I got 96s and 100s.

The new branch was slower in all cases, but the influence of other factors is so 
strong that it is hard to analyze this. From looking at the code, I saw no obvious way 
to make it faster. I don't really know how to proceed.

You can see who'll get notified about comments by hovering over the avatars in 
the top right corner of changeset pages.

I added some comments on bitbuckets to the two latest commits; not sure if you
get notified of these.

> I removed support for axioms and conditional effects. Should I repeat the
> experiment?

You removed them for the domain-abstracted task, right? I don't think there was
anything in the experiment that used them, so I don't think we need to rerun the
experiment.

FWIW, I think xzgrep can be used to grep in compressed files. (It seems to be
available on maia, but I haven't tested it.)

The domains that natively have derived predicates are:
optical-telegraphs philosophers psr-middle psr-large.

The others are compiled from universal conditions; good to test these too, but
they are quite trivial and shouldn't tax the axiom evaluator much. The filtered
experimental results bear this out: three of these four domains are among the
worst four in the total time comparison, and the fourth one is also negative.

Looking at psr-large, runtime is sometimes about 20% larger (e.g. 94 => 113
seconds, 700 => 826 seconds), although there are also improvements sometimes.
I'm not sure what to make of this -- is this bad enough to look at speed a bit more?

I removed support for axioms and conditional effects. Should I repeat the 
experiment?

> If we want to find all domains where the translator introduces derived
> variables, "grep -l begin_rule" on the output.sas files is a safe way.
Unfortunately, the output.sas files are packed and in non-descriptive runs-*/* 
directories. Anyway, I ran a script to test the files from the "satisficing" 
suite and the only tasks with axioms were from the 9 domains:
  assembly
  miconic-fulladl
  openstacks
  openstacks-sat08-adl
  optical-telegraphs
  philosophers
  psr-large
  psr-middle
  trucks
(which is a subset of the "satisficing_adl" suite).

The filtered report now only shows these domains.

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue660-v1-issue660-base-issue660-v1-
compare-filtered.html

Yes, it seems that domain-abstracted tasks should (at least for now) not support
axioms or conditional effects.

It looks like we don't have a plugin for domain-abstracted tasks yet; otherwise
this should also be mentioned in the documentation for users.

> I don't see how a domain-abstracted task could work correctly with axioms with
> the current implementation.
I didn't consider the issue in your example. This looks like a larger problem that 
is not the focus of this issue. Should I remove the overrides again and add an 
exception if the class is used with axioms? Since we didn't have (proper) support 
for axioms in this class before, we wouldn't lose anything.


> BTW, why are some tasks in namespace "tasks" and others in namespace 
"extra_tasks"?
For "historical reasons" :-). I think Jendrik fixed this in different unmerged issue 
branch already.

> By the way, how does the domain abstraction handle conditional effects?
> Doesn't the same problem exist there?
Yes, I think it does. Should I also add a check/exception for that?

ADL and derived predicates are unrelated features, except that we compile away
certain ADL features by introducing derived predicates. I think the domains that
receive derived predicates through compilation are the less interesting domains
in this context, but they are not fully without interest.

"grep -l :derived" in the domain files is a good way to identify domains that
naturally have derived predicates.

If we want to find all domains where the translator introduces derived
variables, "grep -l begin_rule" on the output.sas files is a safe way.

> Regarding the experiment, I'd be interested in a set of experiments that only
> considers the domains that use axioms.

I don't know the domains well enough to know which ones have axioms. If I remember correctly, 
there are also domains where only a subset of tasks have axioms after preprocessing. I found 
that lab has a suite "satisficing_adl"; is this the set you are interested in? Here is the 
filtered report:

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue660-v1-issue660-base-issue660-v1-compare-
filtered.html

By the way, how does the domain abstraction handle conditional effects?
Doesn't the same problem exist there?

For example, let's say we have a "negative" (in the sense of doing something
bad) conditional effect that is triggered if v_1 = 1. If we apply a domain
abstraction to combine various values of v_1, will it now be triggered for all
of the original values? This would also mean we no longer have an
overapproximation, potentially making a solvable problem unsolvable.

I don't see how a domain-abstracted task could work correctly with axioms with
the current implementation, in the sense that abstractions should be
overapproximations.

Let's say we have a derived variable v_1 and a non-derived variable v_2.
Let's say that v_1 is 0 by default and set to 1 if v_2 = 3.
Let's say we want to apply a domain abstraction to combine all values of v_2
into one value.
Let's say our goal is v_1 = 0.
In the abstraction, this goal would become unreachable.

BTW, why are some tasks in namespace "tasks" and others in namespace "extra_tasks"?

Thanks for the review, I made the suggested changes.

> Can you provide a list of all the task classes we have and how we
> would like to handle axioms for them?

RootTask:
accesses the globally stored axioms layers. In the future, this
should become an ExplicitTask and store this information locally.

DelegatingTask:
base class for several other transformations. Unless a method is
overridden in a derived class, this is the identity transformation,
so it also should just forward the request for axiom layers.

CostAdaptedTask:
only changes the operator costs, which should not affect axioms at all.

ModifiedOperatorCostsTask:
only changes the operator costs, which should not affect axioms at all.

ModifiedGoalsTask:
only changes the goal conditions, which should not affect axioms at all.

DomainAbstractedTask:
only changes the domain of each variable (not the number of variables
or their order) by mapping the original domain to a new domain for each
variable. I am less sure about this (see msg5461) but I think this means
that the axioms layers are unchanged. If the parent task has default
value x for variable X then this default value corresponds to some new
value alpha_X(x) where alpha_X is the abstraction function for
variable X.

Regarding the experiment, I'd be interested in a set of experiments that only
considers the domains that use axioms.

I've had a look at the code. After our meeting, I've given up resistance to
exposing the axiom layers to the user (the users have a right of gettings access
to all parts of the task definition).

The main issue I see with the current code is that I'm not sure the treatment of
axioms in the derived Task classes has been thought through. I think we need to
consider each of them individually and clarify what their impact on axioms
should be. Can you provide a list of all the task classes we have and how we
would like to handle axioms for them?

Thanks. I ran experiments that look very noisy, but overall performance looks comparable. I was 
surprised about how different the runtimes can be, but this happens in domains with and without 
axioms, so I assume that this is independent of the patch. Should we merge this?


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

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue660-v1-total_time-lama-first.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue660-v1-total_time-eager_greedy_cea.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue660-v1-total_time-lazy-greedy-ff.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue660-v1-total_time-eager_greedy_cg.png

I had a look and left a few minor comments. The patch looks good.

I created a pull request on bitbucket. Jendrik, could you have a look? In 
particular, I would be interested in whether my implementation for domain 
abstractions is correct. I assumed that axiom levels stay the same, but axiom 
default values have to be translated.

https://bitbucket.org/FlorianPommerening/downward-issues/pull-
requests/21/issue660/diff

Experiments are on their way.

Axiom layers and axiom default values should be supported through the task 
interface. After that, the axiom evaluator can be switched to the task interface.