Issues reported by others that fall under this:

issue1029
issue1030

During the last sprint we addressed the issues that Clemens summarized and those that were still open after merging Patrick's pull request (from issue1030) in
https://github.com/aibasel/downward/pull/247 and issue1177 and issue1178.

- Conditions are now allowed to be empty.
- We check more thoroughly if predicates and terms that are used are defined 
  somewhere.
- We print a warning if a predicate is defined in the predicates-section that 
  uses the same name as a type (even if the predicate is not unary). We do not
  raise an error here because the PDDL definition of McDermott et al. (TR
  1998) treats types just as unary predicates and, as far as I remember from
  our discussion about this, because some benchmarks rely on this.
- Checking if the arity is correct now also happens for problem files and not 
  only for domain files.
- "number" cannot be defined as a (new) type in the types section because it is 
  reserved as a special type.
- The keyword "either" is allowed only in the predicates section because the 
  storage domain uses it. It is not supported though.
- We check for duplicate action names.
- The end of a domain file is now better handled. Before, everything except 
  "metric" coming after the goal was ignored. Now nothing except the metric
  section is allowed after the goal.
- An empty list before a type declaration raises a warning (e. g. writing "- 
  sometype" instead of "someobject - sometype"). This should probably be
  considered malformed PDDL but the woodworking domain contains some problem
  files where this happens. Raising a warning instead of an error is a
  compromise such that this domain can still be parsed but the user also gets a
  notification.

Salomé ran some experiments after these changes and posted the following on Discord:

https://ai.dmi.unibas.ch/_experiments/ai/downward/issue220/data/experiments-v5-eval/experiments-v5-issue220-base-issue220-v5-compare.html#summary

The sum of all parsing now goes up 11 seconds from 170s to 181s. Note that this is only a small part of the overall translator; the overall translator time sum is roughly 16500s.

A relative scatterplot shows that on bigger problems, parsing is up to 10% slower now:

https://ai.dmi.unibas.ch/_visualizer/?c=eJyNUDtuwzAMvYqhuZacFF1yju4GHbMOAVsSKMqfBLl7qAaB204d9b58uhn0woSpHSmJOVWmopQyHo9N3UHCWhh8GkGwDn7cqp2dP_5w5q0yA4ccu63k9GEC8gVkVA3NqKhwRkVWEC3tshTMvGICt0ITthE4kR-KdU10LZpD0zT63P7r2377IoeILGXmF43f5otITCfngGw_kc2edK49X1yLq2o1z0tS1vVh8Qtw717LXQ8C7oeqfAXOMLq9xgqwXa_P9TGwfG6x1L4rMKOeGXw54mCbIjnnJGFqJ_LtQj4pc7vvMAwD41BmPpn7A5NWlxA%3D



Looking at the issues mentioned in this one, issue813 and maybe also issue215 seem to be handled now. Should we resolve them?

At some point in the process of issue1030 I went through the issue tracker to find more issues related to the translator error reporting. We decided it makes sense to collect them all in one place and discussed that this might be a good place for that. By doing so, I'm also changing this issue to have priority meta, so this now serves to have an overview about all the related issues in one place from now on. So here's a summary of what I've found:

Issues that are already dealt with after merging issue1030:
- too many parentheses (issue215)
- use undefined predicate name (issue215)
- first token in domain is not "define" (issue1029)
- empty conditions (issue1030)
- undeclared predicates (issue215 and issue220)
- "either" disallowed (issue220 and issue297)
- predicates with wrong arity (issu294)

Next, here is a list of things mentioned in other issues that are not yet addressed:
- disallow predicates with the same name as object types (issue171 and issue293)
- disallow duplicate action names (issue215 and issue430)
- use of object name instead of variable (issue220)
- "numeric" should be a reserved term and in particular not allowed for object types (issue220)
- complain about undefined objects (issue813)
- complain about using variables undeclared in parameters (no issue)

There's also issue472 that is related to this one, but I think it is sufficiently independent to not be considered part of this meta issue.

When we merged issue1030, we left many open pull request comments on the table. It would be nice to address them eventually. See

    https://github.com/aibasel/downward/pull/155

for more details. Make sure to display the changes for the file where the diff is too large to be shown by default to see everything. The whole list includes comments by Victor, Salomé, Florian, Clemens and me (and possibly others).

Added reference to issue1029 to summary.

Error reporting should also mention more about the context in which the error
occurs, such as a line number and perhaps a few lines of context in the PDDL
file. See message by David Epstein on Fast Downward list 09.06.2019 10:08 (CEST).

I'm attaching another example where our error reporting should be improved:


...
Completing instantiation...
Traceback (most recent call last):
  File "./translate/translate.py", line 578, in <module>
    sas_task = pddl_to_sas(task)
  File "./translate/translate.py", line 441, in pddl_to_sas
    reachable_action_params) = instantiate.explore(task)
  File "/home/abdon/downward/src/translate/instantiate.py", line 77, in explore
    return instantiate(task, model)
  File "/home/abdon/downward/src/translate/instantiate.py", line 57, in instantiate
    fluent_facts, type_to_objects)
  File "/home/abdon/downward/src/translate/pddl/actions.py", line 137, in
instantiate
    objects_by_type, effects)
  File "/home/abdon/downward/src/translate/pddl/effects.py", line 129, in
instantiate
    self._instantiate(var_mapping, init_facts, fluent_facts, result)
  File "/home/abdon/downward/src/translate/pddl/effects.py", line 137, in
_instantiate
    self.literal.instantiate(var_mapping, init_facts, fluent_facts, effects)
  File "/home/abdon/downward/src/translate/pddl/conditions.py", line 300, in
instantiate
    raise Impossible()
pddl.conditions.Impossible


The cause of this is apparently that some condition is written as "(open ?door)"
instead of the correct "(open ?d)".

> As another example where better error reporting would be useful, see issue297. I
> think the underlying problem there is that the input uses the "either"
> construct, which we do not support.

Actually no, there is another problem. Still, we should complain about "either"
if we don't support it.

As another example where better error reporting would be useful, see issue297. I
think the underlying problem there is that the input uses the "either"
construct, which we do not support.

> Will open a separate issue for this.

issue293

> It also allows using types as predicates (which is correct according to the
> PDDL 1 specification), but does not check if this clashes with actual
> predicates, as is e.g. the case in the Schedule-ADL domain (not in our
> repository -- we renamed the type "temperature" to "temperature-type" to
> avoid the name clash). In case of such clashes, we typically get a very
> uninformative error message about a tuple index being out of range.

I just got another error report about this behaviour. Will open a separate issue
for this.

The translator accepts quite a few things that are invalid PDDL. Test cases
include the domains mentioned in issue218 before the issue was fixed.

In particular, the translator accepts:

 * predicates that are not declared
 * an object type named "number", which should be reserved for actual
   numeric quantities

It also allows using types as predicates (which is correct according to the PDDL
1 specification), but does not check if this clashes with actual predicates, as
is e.g. the case in the Schedule-ADL domain (not in our repository -- we renamed
the type "temperature" to "temperature-type" to avoid the name clash). In case
of such clashes, we typically get a very uninformative error message about a
tuple index being out of range.