Support subtraction in preconditions in domains.
MichaelJFishman opened this issue · 9 comments
Is your feature request related to a problem? Please describe.
The DomainParser cannot seem to handle (- (function_name object_name) 1) as part of a precondition.
Here is an example domain showing this issue. (In this domain, the arithmetic in the precondition isn't needed since we compare to a constant 1, but in my actual domain of interest we compare to another numeric function).
(define (domain cant-subtract-in-precondition)
(:requirements :equality :typing :fluents :negative-preconditions :universal-preconditions :existential-preconditions)
(:types
agent - object
)
(:functions
(x ?l - agent)
)
(:action move-south
:parameters (?ag - agent)
:precondition (= (- (x ?ag) 1) 1)
:effect (and (decrease (x ?ag) 1))
)
)Describe the solution you'd like
Parse the subtraction correctly.
Describe alternatives you've considered
As a workaround, I can do (+ (function_name object_name) (- 1))
Additional context
Add any other context or screenshots about the feature request here.
@marcofavorito @francescofuggitti Can I get your take on this one? I've banged my head against the proverbial Lark wall for quite some time now, and am convinced that our choice of parser (LALR) prohibits us from handling both (- 3) and (- 3 1) because of the lookahead depth. If we were to drop one, I'd probably drop the unary use of the operator.
I should note that I went as far as having a new symbol (NEGATE) introduced, and a unary_op rule put in place. It just doesn't work for one or the other of the two uses above. (- f_exp) can be done with (* f_exp -1), which seems easy enough.
Thoughts? You can see the proposed changes over at #104
... and am convinced that our choice of parser (LALR) prohibits us from handling both
(- 3)and(- 3 1)because of the lookahead depth.
Hi @haz , what about this:
diff --git a/pddl/parser/domain.lark b/pddl/parser/domain.lark
index 6703c10..629ae9e 100644
--- a/pddl/parser/domain.lark
+++ b/pddl/parser/domain.lark
@@ -77,7 +77,7 @@ constant: NAME
f_exp: NUMBER
| LPAR binary_op f_exp f_exp RPAR
| LPAR multi_op f_exp f_exp+ RPAR
- | LPAR MINUS f_exp RPAR
+ | LPAR MINUS f_exp+ RPAR
| f_head
f_head: NAME
diff --git a/pddl/parser/domain.py b/pddl/parser/domain.py
index e08bfdd..fc58d74 100644
--- a/pddl/parser/domain.py
+++ b/pddl/parser/domain.py
@@ -368,6 +368,9 @@ class DomainTransformer(Transformer):
return args[0]
op = None
if args[1] == Symbols.MINUS.value:
+ if len(args[2:-1]) == 1:
+ return Negate(args[2])
+ # else as usual
op = Minus
if args[1] == Symbols.PLUS.value:
op = Plus
We disambiguate the use of - by considering the number of arguments; if the number of the operands is one, then - should be interpreted as a negation; otherwise, as a minus.
In fact, I think we should have a class that represents the expression (- <f-exp>), as e.g. this PDDL spec:
Ah, I see. Just leave it up to the post-grammar parsing.
Allowing any number of arguments to a minus is kind of risky...put in a test to force it to explicitly fail. How's it look now?
Ah, I see. Just leave it up to the post-grammar parsing.
Allowing any number of arguments to a minus is kind of risky...put in a test to force it to explicitly fail. How's it look now?
Allowing any number of arguments was the same approach taken for the other operators, and I don't see why the minus should be considered different in that respect. We have to decide which route to take for the numeric operators: either (i) allowing any number of operands (to be interpreted as left-associative), or (ii) just two operators (ending up in a binary syntax tree).
I also think that, for compatibility with "standard" PDDL (which I assume marries option (ii)), we should configure the printer class to generate the PDDL spec in binary form. I could not spot examples in which option (i) is used in the PDDL book.
Overall, supporting both (i) and (ii) might surprising our users, and can add some complexity to the maintenance.
Ya, I think sticking to the available bnf makes sense. This means handling minus in two ways, and having 3+ operands only work for addition and multiplication.
Do we want to maintain the PDDL fed to the parser as the same spat back out? Current proposal has the single operand minus converted to the multiplication of -1 and the f_exp.
Ya, I think sticking to the available bnf makes sense. This means handling minus in two ways, and having 3+ operands only work for addition and multiplication.
Do we want to maintain the PDDL fed to the parser as the same spat back out? Current proposal has the single operand minus converted to the multiplication of -1 and the f_exp.
Thank you very much @haz for your PR! My preference is to have a specific class that represents the (- <f-exp>) expression because it is syntactically different from <f-exp> * -1, and this information is lost after parsing. But I am OK with the current solution, too, so I think we can merge it. I can handle it in another PR if we agree with the above.
Hi all,
sorry for not getting back to you sooner. I agree with what you already said about sticking with the current BNF as much as possible. Thank you, @haz, for handling the issue.
My preference is to have a specific class that represents the (- ) expression because it is syntactically different from * -1, and this information is lost after parsing.
Aren't problems (e.g., on the printing side?) of not having this already implemented now? In case not, it's totally fine as it is, but we might want to consider it in the future if we want to support additional features requiring the info being kept.
Let's leave this open for a full fix. I'm partway through.
One issue that just arose is the inheritance. Functions inherit from Atomic, which inherits from Formula. This means you can start using &, |, etc on functions. This isn't intended, right?
More broadly, there seems to be little in the way of testing cases that should fail. I added one to test if a 3 operand minus failed in the right way, but it is kind of a hack (substring match on the error message). There a well crafted example that tests for intended failures?