Already done.

OK, the new code is definitely not worse than the old. If anything, it's better.
Since this is already merged, I'm marking it as resolved.

Silvia, if you haven't done yet, you can inform the original reporter of this
problem that this is fixed.

I have run another experiment, preprocessing all domains and parsing much more 
attributes. Results are attached.

I chose to parse the output instead of checking the returncode, because 
that makes things easier. Now two additional tables are printed, called 
preprocessor_error and translator_error. I will upload a new table later.

Catching a MemoryError doesn't usually do much good.

In general, checking for particular kinds of errors will often lead to
incomplete checking since we don't know for sure which errors to expect.
Instead, the scripts should verify *success* of the translator (and preprocessor
etc.). For this, it should be enough to check the exit code. It it is nonzero,
there is an error.

Additionally, it might be useful to identify particular kinds of errors to give
nicer reports, but that's not strictly necessary.

How would you like to log translator errors? They all seem to be of the type 
MemoryError. Do you think it makes sense to catch that error in the translator 
code or should the scripts parse the error files for "MemoryError"?

As far as I can see, the differences are due to arbitrary differences in the
translator output (see issue22).

Still, also for translator changes in the future, it would be interesting to do
a detailed comparison at the translator/preprocessor level, *without running the
search code at all*. This should be pretty fast. We should also use the complete
suite, not just the STRIPS problems. I would suggest to run the union of the old
ALL and LMCUT suites, plus the IPC-2008 domains, seq-sat and seq-opt.

What should be measured:

 * translator time
 * all detailed translator timings (lines of the form "XXX.YYYs CPU")
 * number of variables, derived variables, facts, operators, axioms and total
   problem size after translating
 * number of variables, derived variables, facts, operators, axioms and total
   problem size after preprocessing
 * number of invariant groups and total invariant group sizes after translating
 * the numbers from the following lines of translator output:
   170 relevant atoms
   141 auxiliary atoms
   311 final queue length
   364 total queue pushes
   13 uncovered facts
   0 implied effects removed
   0 effect conditions simplified
   0 implied preconditions added
   0 operators removed
   38 propositions removed


Total problem size can be measured as the total number of tokens in the
output.sas/output file (sum(len(line.split()) for line in lines)).

Number of invariant groups is the second line in the "all.groups" file.

Total invariant group sizes is the sum over all numbers that follow a "group"
line in the "all.groups" file.

OK, started looking. First observations: on runs 00762, 00763, 01608 and 01609
the translator failed.

This is kind of related to issue137, but not quite the same thing. Translator
errors are not fatal errors in the same sense that script errors are, but
certainly severe errors that should be brought to our attention.

> maybe the problem is that the older revision didn't have
> "downward" renamed to "src" yet?
That was the problem. Checking out two versions and a meld showed that the 
correct versions should have been compared.

I'll have a look at the translator output later and see if I can make sense of it.

Regarding the hg diff, maybe the problem is that the older revision didn't have
"downward" renamed to "src" yet? I don't know if there is a way to follow the
change across the diff automagically; in such a situation I would just check out
both versions of the code and run a regular diff or meld.

I have run a test comparing the two different translator versions before and 
after the change (the tests use the current preprocessor and search code). The 
right column actually contains the results for the earlier version.

I think the results show a bigger impact than expected. I hope this doesn't mean 
there are bugs in the new-scripts code... 
Also I hope I chose the right revisions. 

hg diff -r a71e8f733599 -r 952811b6757e src/translate/

Does not give me meaningful results (It tells me that all files in that 
directory have been added between the two revisions). What am I doing wrong 
here?

Marking as resolved. Will reopen if the testing uncovers problems, which would
be very unexpected in this case.

The testing will have to wait until habakuk gets mercurial installed 
(bash alias does of course not work with subprocess.call). First tests 
show no difference however.

Code review sent by email.

After the next update, can you also test if this fixes the problem with the
attached KEGG.zip and do a before/after comparison for the translator?

(I doubt this would change much, but it'd be good if we made a habit out of
doing before/after comparisons for the translator, automated as much as possible.)

It'd be interesting to know

 * if all outputs remain the same, and
 * whether there are any significant changes in terms of runtime
   (can't easily measure memory at the moment, unfortunately).

I have fixed the issue by making the algorithm iterative. It took a 
little longer than I expected, because it was not easy to keep the code 
readable (Probably that's why recursion was used in the first place). It 
looks better now, I think and I could not resist adding some whitespace 
and comments...

I have pushed the code to bitbucket. My username is digitaldump and the 
repo is called downward. I made the repo public just now, so I think 
pulling should work.

That's the easiest way to do it. Erez set up a bitbucket account for that, which
I think is a good way to host it. My username there is malte.

BTW: How do you get my changes? Do I have to push the repo to a public 
place?

Let me have a shot at this. I'll try to follow the hg workflow.

Jendrik and I have looked into this issue. It looks like the fix is simple: In
build_model.ProductRule._fire, either use a completely iterative algorithm
instead of the current recursive one, or at least avoid recursion in the case
where len(self.atoms_by_index[position]) == 1. (The case where it is == 0 does
not need to be covered since then _fire cannot be triggered due to the
"empty_atom_list_no" logic. It might be worth asserting nonemptiness of
self.atoms_by_index[position], though, to verify that this logic works as expected.)

Jendrik, do you want to fix this one or should I do it? (It'd be no big deal for
me to do it myself; looks straight-forward enough.)

Erez has moved the new bug to issue49.

Erez, this is indeed a second bug. Your analysis makes sense. Can you open a
separate issue and attach an example input and your patch (svn diff) if you
still have it around? I'll fix it in a similar way to what you suggested, but
probably not quite identical as there are some subtle issues there. You can
assign to me.

This is another bug from the same domain, so I'm adding it to this issue,
although it might be a different bug.

When translating the attached domain and problem files, the following error occurs: 

Traceback (most recent call last):
  File "/home/batman/code/downward/trunk/downward/translate/translate.py", line
383, in <module>
    sas_task = pddl_to_sas(task)
  File "/home/batman/code/downward/trunk/downward/translate/translate.py", line
301, in pddl_to_sas
    task.init, goal_list, actions, axioms, task.use_min_cost_metric)
  File "/usr/lib/python2.5/contextlib.py", line 33, in __exit__
    self.gen.throw(type, value, traceback)
  File "/home/batman/code/downward/trunk/downward/translate/timers.py", line
32, in timing
    yield
  File "/home/batman/code/downward/trunk/downward/translate/translate.py", line
301, in pddl_to_sas
    task.init, goal_list, actions, axioms, task.use_min_cost_metric)
  File "/home/batman/code/downward/trunk/downward/translate/translate.py", line
244, in translate_task
    goal_pairs = translate_strips_conditions(goals, strips_to_sas, ranges,
mutex_dict, mutex_ranges).items()
  File "/home/batman/code/downward/trunk/downward/translate/translate.py", line
71, in translate_strips_conditions
    conditions, mutex_dict, mutex_ranges) is None:
  File "/home/batman/code/downward/trunk/downward/translate/translate.py", line
42, in translate_strips_conditions_aux
    for var, val in dictionary[atom]:
KeyError: <pddl.conditions.Atom object at 0xb7adb60c>

I did some digging around, and found that the atom that causes this problem is
non-fluent (does not change), and is therefore not in the dictionary, but it
does appear in the goal.
By adding a check if the atom is in the dictionary before the line that causes
the exception I was able to complete translation, but I am not sure if this is
correct.

Reclassified this as a bug.

In build_model.py in the translator, some inputs (see attached) create a
"maximum recursion depth reached" error. Possible fixes: change the way the
greedy-join tree is generated, so it doesn't degenerate into a list (if that's
the problem), or turn recursive algorithm into an iterative one.