PDDL has a lot of cases where a list of (sometimes typed) parameters are represented in the form (?var1 - type1 ?var2 - type2 ...). Throughout the codebase, several classes implement different routines to generate this same string format from a collection of Variable objects.
For consistency and ease of maintenance/modification, pddl should choose a single universal implementation for formatting typed parameter lists in PDDL syntax and modify all relevant classes to invoke this shared code instead of defining their own class-specific methods. The function _typed_parameters from pddl/helpers/base.py is a perfect candidate for this.
In the discussion on the recently merged #71, I remarked that although the classes ForallCondition and Action produce identical parameter strings in their __str__ methods, they use different code to do so. ForallCondition.__str__ (inherited from QuantifiedCondition^1) constructs its parameter list via a list comprehension that uses a locally defined function build_tags(tags) to generate each variable's (possibly empty) type annotation individually. In contrast, the :parameters portion of Action.__str__^2 imports _typed_parameters from pddl/helpers/base.py to handle the entire Optional[Collection[Variable]] -> str process.
I'm of the opinion that the latter approach is preferable for three main reasons:
using a shared implementation means consistent PDDL across classes and updates,
wrapping the parameter list translation in a separate function limits the chance of unintended interactions with other steps of __str__, and
_typed_parameters seems to have been specifically designed to handle generating valid PDDL parameter lists, including handling the absence of types and empty parameter lists.
Accordingly, since #71, Forall.__str__'s parameter string is produced via _typed_parameters as well.
Suggested fix
For consistency and ease of maintenance/modification, pddl should designate pddl/helpers/base.py::_typed_parameters as the single universal implementation for formatting typed parameter lists in PDDL syntax and modify all relevant classes to invoke this shared code instead of defining their own class-specific methods. Specific action items include the following:
QuantifiedCondition should be modified to use the same _typed_parameters-based implementation as Action and Forall.
If other methods of generating PDDL for typed parameter lists exist elsewhere in the codebase, they should be replaced as necessary with _typed_parameters.
Future updates that add new PDDL objects with parameter lists (e.g. exists effects) should opt to use _typed_parameters in their __str__ methods to maintain this consistency (and should also reference fix (1) from #71 when writing their parsing rules, see "§Universal parameter list parsing function" below).
Additional comments
PDDL-agnostic Python objects
It's worth considering whether it should be the job of __str__ methods to output the canonical PDDL representation. One reason for hesitation is that there is not always one "correct" representation bereft of context. For example, Variable.__str__^3 (from pddl/logic/terms.py) prints ?varname regardless of whether or not its type has been defined. This is desired behavior when the variable occurs as an argument of a ground predicate, e.g. preconditions: (at ?loc), but not when it is being declared, e.g. as an action parameter parameters: (?loc - location).
It could be argued that all consideration of "PDDL syntax" should be left to the parser/formatter, and that the Python objects should be as agnostic as possible to how they will be represented outside of Python. In this case, the point of this issue remains, but it should be the formatter that uses _typed_parameters instead of each class's __str__ method.
Universal parameter list parsing function
A similar thing is happening indomain.py where parameter lists are occur in several different contexts that all interpret the tokens as a collection of Variable objects, but there is not a unified implementation. This actually was the cause of bug (1) in #71 , where pddl/parser/domain.py::DomainTransformer::c_effect was not casting the parameters of forall effects to Variable objects as they are for non-effect forall conditions via gd_quantifiers and actions via action_parameters.
In domain.lark, we can see that these all invoke the typed_list_variable^4 rule for their parameter lists. It seems as though it could be useful to have casting parameters to Variable objects handled in typed_list_variable instead of in each individual context for the same reasons as above.
Footnotes
```python
...
def __str__(self) -> str:
"""Get the string representation."""
def build_tags(tags):
if len(tags) == 0:
return ""
return f" - {' '.join(tags)}"
var_block = " ".join([f"{v}{build_tags(v.type_tags)}" for v in self.variables])
return f"({self.SYMBOL} ({var_block}) {self.condition})"
...
```
```python
...
def __str__(self):
"""Get the string."""
operator_str = "(:action {0}\n".format(self.name)
operator_str += f" :parameters ({_typed_parameters(self.parameters)})\n" # <<< parameter string
if self.precondition is not None:
operator_str += f" :precondition {str(self.precondition)}\n"
if self.effect is not None:
operator_str += f" :effect {str(self.effect)}\n"
operator_str += ")"
return operator_str
...
```
```python
def typed_list_variable(self, args):
"""
Process the 'typed_list_variable' rule.
Return a dictionary with as keys the terms and as value a set of types for each name.
:param args: the argument of this grammar rule
:return: a typed list (variable)
"""
return self._typed_list_x(args)
```
Tl;dr
PDDL has a lot of cases where a list of (sometimes typed) parameters are represented in the form
(?var1 - type1 ?var2 - type2 ...)
. Throughout the codebase, several classes implement different routines to generate this same string format from a collection ofVariable
objects.For consistency and ease of maintenance/modification,
pddl
should choose a single universal implementation for formatting typed parameter lists in PDDL syntax and modify all relevant classes to invoke this shared code instead of defining their own class-specific methods. The function_typed_parameters
frompddl/helpers/base.py
is a perfect candidate for this.(issue requested by @haz here)
Context
In the discussion on the recently merged #71, I remarked that although the classes
ForallCondition
andAction
produce identical parameter strings in their__str__
methods, they use different code to do so.ForallCondition.__str__
(inherited fromQuantifiedCondition
^1) constructs its parameter list via a list comprehension that uses a locally defined functionbuild_tags(tags)
to generate each variable's (possibly empty) type annotation individually. In contrast, the:parameters
portion ofAction.__str__
^2 imports_typed_parameters
frompddl/helpers/base.py
to handle the entireOptional[Collection[Variable]] -> str
process.I'm of the opinion that the latter approach is preferable for three main reasons:
__str__
, and_typed_parameters
seems to have been specifically designed to handle generating valid PDDL parameter lists, including handling the absence of types and empty parameter lists.Accordingly, since #71,
Forall.__str__
's parameter string is produced via_typed_parameters
as well.Suggested fix
For consistency and ease of maintenance/modification,
pddl
should designatepddl/helpers/base.py::_typed_parameters
as the single universal implementation for formatting typed parameter lists in PDDL syntax and modify all relevant classes to invoke this shared code instead of defining their own class-specific methods. Specific action items include the following:QuantifiedCondition
should be modified to use the same_typed_parameters
-based implementation asAction
andForall
._typed_parameters
.exists
effects) should opt to use_typed_parameters
in their__str__
methods to maintain this consistency (and should also reference fix (1) from #71 when writing their parsing rules, see "§Universal parameter list parsing function" below).Additional comments
PDDL-agnostic Python objects
It's worth considering whether it should be the job of
__str__
methods to output the canonical PDDL representation. One reason for hesitation is that there is not always one "correct" representation bereft of context. For example,Variable.__str__
^3 (frompddl/logic/terms.py
) prints?varname
regardless of whether or not its type has been defined. This is desired behavior when the variable occurs as an argument of a ground predicate, e.g.preconditions: (at ?loc)
, but not when it is being declared, e.g. as an action parameterparameters: (?loc - location)
.It could be argued that all consideration of "PDDL syntax" should be left to the parser/formatter, and that the Python objects should be as agnostic as possible to how they will be represented outside of Python. In this case, the point of this issue remains, but it should be the formatter that uses
_typed_parameters
instead of each class's__str__
method.Universal parameter list parsing function
A similar thing is happening in
domain.py
where parameter lists are occur in several different contexts that all interpret the tokens as a collection ofVariable
objects, but there is not a unified implementation. This actually was the cause of bug (1) in #71 , wherepddl/parser/domain.py::DomainTransformer::c_effect
was not casting the parameters offorall
effects toVariable
objects as they are for non-effectforall
conditions viagd_quantifiers
and actions viaaction_parameters
.In
domain.lark
, we can see that these all invoke thetyped_list_variable
^4 rule for their parameter lists. It seems as though it could be useful to have casting parameters toVariable
objects handled intyped_list_variable
instead of in each individual context for the same reasons as above.Footnotes