Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,6 @@ dist/

# Mypy specific
.mypy_cache/

# Vim
*.swp

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a crash recovery file on Vim, right? In that case, I really don't think this should be in the .gitignore. Just don't commit it, and once it serves its purpose ideally remove it from your filesystem.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vim creates .swp files whenever a file is opened; basically if you git status while vim is still open it'll show a bunch of .swp files. I can remove it from the .gitignore

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I actually think it is a good idea to include in the ..gitignore. Otherwise, these swp files show up in the status checks and "untracked". Technically this is harmless, but in practice, this is annoying and distracting.
So putting *.swp and *~ in the .gitignore file is good practice.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, personally, I've never had this issue while using Vim, and am kind of of the belief that a .gitignore should only include what's relevant to the project itself, and is independent of the tooling used by developers, but if you really feel like this is fine, I'm also not too against it.

It does feel like just using Git's core.excludesfile would be enough though, but I'm not a very frequent Vim user anymore.

51 changes: 48 additions & 3 deletions src/pddlsim/_asp.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from abc import ABC, abstractmethod
from collections.abc import Callable, Generator, MutableMapping, Sequence
from collections.abc import Callable, Generator, MutableMapping, Sequence, Mapping
from dataclasses import dataclass, field
from enum import StrEnum
from itertools import chain
Expand All @@ -15,6 +15,7 @@
Condition,
Domain,
EqualityCondition,
ForallCondition,
Identifier,
NotCondition,
Object,
Expand Down Expand Up @@ -454,6 +455,7 @@ def argument_to_asp(argument: Argument) -> ArgumentAST:


def action_definition_asp_part(
problem: Problem,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really rather you not add the problem to the action definition, as this basically destroys caching, which we currently make use of. You don't need to do this! You can encode universal quantification just fine without explicitly mentioning objects from the problem, and making a bunch of and conditions. I'll elaborate on this when I summarize my review.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree that this is disgusting -- to be perfectly transparent this is the first time I'm dealing with ASP so I think I didn't solve the problem in a good way :)

I'll fix it to match your implementation

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as of latest commit but ended up doing a rewrite

action_definition: ActionDefinition,
variable_id_allocator: IDAllocator[Variable],
object_id_allocator: IDAllocator[Object],
Expand All @@ -478,9 +480,52 @@ def action_definition_asp_part(
],
)

# Specify the precondition over the parameters
# Preprocess precondition to replace Foralls with series of Ands
def _expand_substitute(
condition: Condition[Argument], substitutions: Mapping[Variable, Object]
) -> Condition[Object]:
"""Fold in to use global information for Forall..."""
match condition:
case AndCondition(subconditions):
return AndCondition(
[
_expand_substitute(subcondition, substitutions)
for subcondition in subconditions
]
)
case OrCondition(subconditions):
return OrCondition(
[
_expand_substitute(subcondition, substitutions)
for subcondition in subconditions
]
)
case NotCondition(base_condition):
return NotCondition(_expand_substitute(base_condition, substitutions))
case EqualityCondition(left_side, right_side):
return EqualityCondition(
substitutions.get(left_side, left_side),
substitutions.get(right_side, right_side),
)
case ForallCondition(loop_var, condition):
new_subs = dict(substitutions)
conditions = []
for param in problem.objects_section:
if param.type == loop_var.type:
new_subs[loop_var.value] = param.value
conditions.append(_expand_substitute(condition, new_subs))
return AndCondition(conditions)
case Predicate(name, assignment):
return Predicate(
name,
tuple(
substitutions.get(argument, argument)
for argument in assignment
),
)

precondition_id = _add_condition_to_asp_part(
action_definition.precondition,
_expand_substitute(action_definition.precondition, {}),
part,
IDAllocator.from_id_constructor(RuleID),
variable_id_allocator,
Expand Down
27 changes: 27 additions & 0 deletions src/pddlsim/ast.py
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,32 @@ def _as_str_without_location(self) -> str:
def __repr__(self) -> str:
return f"(= {self.left_side!r} {self.right_side!r})"

@dataclass(frozen=True)
class ForallCondition[A: Argument]:
"""Represents an forall predicate (forall (arg) condition) in PDDL."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "a", not "an", and in parnetheses, write forall, for consistency with other documentation.


loop_var: Typed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this called loop_var? Where is the loop here? Also try to not use shortenings. quantification_variable, or just variable is better here. Also what is Typed? We have a Typed[T] but not Typed here. You probably meant somethingf like Typed[Identifier], although to be entirely honestly, you'll likely want to allow quantification over multiple variables (with the necessary grammar changes), so I would look to ActionDefinition for inspiration.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed renamed to (variable).

Type list[Typed[Variable]]

condition: "Condition[A]"

def _validate(
self,
parameters: Parameters,
objects: Mapping[Object, Type],
domain: "Domain",
) -> None:

scope_dict = dict(parameters._items)
scope_dict[self.loop_var.value] = self.loop_var.type
self.condition._validate(scope_dict, objects, domain)

@override
def _as_str_without_location(self) -> str:
return "forall predicate"

@override
def __repr__(self) -> str:
return f"(= {self.left_side!r} {self.right_side!r})"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a remnant of copying from EqualityCondition. Please change this. A proper __repr__ implementation is actually very key to allow agents to work with your universal conditions (via the RSP protocol).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.



class _SerializedPredicate(TypedDict):
name: Any
Expand Down Expand Up @@ -735,6 +761,7 @@ def __str__(self):
| OrCondition[A]
| NotCondition[A]
| EqualityCondition[A]
| ForallCondition[A]
| Predicate[A]
)
"""Represents a condition in PDDL, over objects, or variables."""
Expand Down
5 changes: 4 additions & 1 deletion src/pddlsim/grammar.lark
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,11 @@ EQUALS_SIGN: "="
| "(" OR_KEYWORD list_{condition{argument}} ")" -> or_condition
| "(" NOT_KEYWORD condition{argument} ")" -> not_condition
| "(" EQUALS_SIGN argument argument ")" -> equality_condition
| "(" "forall" "(" VARIABLE "-" _type ")" condition{argument} ")" -> forall_condition

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably also allow quantification over all objects (no type specified). You should probably create a parameterized parser (like say typed_list{item}) for this purpose, to be as general as possible.

| predicate{argument}

//| "(" "forall" "(" VARIABLE "-" _type ")" "when" "(" condition{argument} ")" condition{argument} ")" -> forall_when_condition

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this comment still here? Is this part of the PR, or not? If not, remove it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


not_predicate{argument}: "(" "not" predicate{argument} ")"
_atom{argument}: predicate{argument}
| not_predicate{argument}
Expand Down Expand Up @@ -109,4 +112,4 @@ _goal_section: "(" ":goal" condition{object_} ")"
GOALS_KEYWORD: ":goals"
goals_section: "(" GOALS_KEYWORD list_{condition{object_}} ")"

problem: "(" "define" _problem_name _used_domain_name [[requirements_section{problem_requirement}]] [objects_section] [action_fallibilities_section] [revealables_section] [initialization_section] (_goal_section | goals_section) ")"
problem: "(" "define" _problem_name _used_domain_name [[requirements_section{problem_requirement}]] [objects_section] [action_fallibilities_section] [revealables_section] [initialization_section] (_goal_section | goals_section) ")"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a diff here? These lines look the exact same. Weird.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an ending newline diff? I don't know...

9 changes: 9 additions & 0 deletions src/pddlsim/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
Effect,
EqualityCondition,
FileLocation,
ForallCondition,
GoalsSection,
GroundedActionSchematic,
Identifier,
Expand Down Expand Up @@ -200,6 +201,14 @@ def equality_condition[A: Argument](
) -> EqualityCondition[A]:
return EqualityCondition(left_side, right_side, location=location)

def forall_condition[A: Argument](
self,
loop_var: A,
loop_type: A,
condition: Condition[A],
) -> ForallCondition[A]:
return ForallCondition(Typed(loop_var, loop_type), condition)

def not_predicate[A: Argument](
self, base_predicate: Predicate[A]
) -> NotPredicate[A]:
Expand Down
22 changes: 17 additions & 5 deletions src/pddlsim/simulation.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
Domain,
Effect,
EqualityCondition,
ForallCondition,
GroundedAction,
Identifier,
NotCondition,
Expand Down Expand Up @@ -76,30 +77,39 @@ def _ground_predicate(


def _ground_condition(
condition: Condition[Argument], grounding: Mapping[Variable, Object]
condition: Condition[Argument], grounding: Mapping[Variable, Object], problem: Problem
) -> Condition[Object]:
"""Fold in to use global information for Forall..."""
match condition:
case AndCondition(subconditions):
return AndCondition(
[
_ground_condition(subcondition, grounding)
_ground_condition(subcondition, grounding, problem)
for subcondition in subconditions
]
)
case OrCondition(subconditions):
return OrCondition(
[
_ground_condition(subcondition, grounding)
_ground_condition(subcondition, grounding, problem)
for subcondition in subconditions
]
)
case NotCondition(base_condition):
return NotCondition(_ground_condition(base_condition, grounding))
return NotCondition(_ground_condition(base_condition, grounding, problem))
case EqualityCondition(left_side, right_side):
return EqualityCondition(
_ground_argument(left_side, grounding),
_ground_argument(right_side, grounding),
)
case ForallCondition(loop_var, condition):
conditions = []
for param in problem.objects_section:
if param.type == loop_var.type:
instance_grounding = dict(grounding)
instance_grounding[loop_var.value] = param.value
conditions.append(_ground_condition(condition, instance_grounding, problem))
return AndCondition(conditions)
case Predicate():
return _ground_predicate(condition, grounding)

Expand Down Expand Up @@ -218,6 +228,7 @@ def _action_definition_asp_parts(
return {
action_definition.name: (
action_definition_asp_part(
self.problem,
action_definition,
variable_id_allocator := IDAllocator[
Variable
Expand Down Expand Up @@ -372,7 +383,7 @@ def apply_grounded_action(self, grounded_action: GroundedAction) -> bool:
}

if not self.state.does_condition_hold(
_ground_condition(action_definition.precondition, grounding)
_ground_condition(action_definition.precondition, grounding, self.problem)
):
raise ValueError("grounded action doesn't satisfy precondition")

Expand All @@ -389,6 +400,7 @@ def apply_grounded_action(self, grounded_action: GroundedAction) -> bool:

return True


def _get_groundings(
self, action_definition: ActionDefinition
) -> Generator[Mapping[Variable, Object]]:
Expand Down
2 changes: 1 addition & 1 deletion src/pddlsim/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def _make_atom_hold(self, atom: Atom[Object]) -> None:
case Predicate():
self._true_predicates.add(atom)
case NotPredicate(base_predicate):
self._true_predicates.remove(base_predicate)
self._true_predicates.discard(base_predicate)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the idea here is to allow non-existent predicates, but this is not something we plan to support. Non-existent predicates will always remain an error, for the foreseeable future.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'm pretty sure you'd have to modify the parser for this anyway, since we currently do validation on all PDDL input.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should've specified the intent; I ran into an error where I was setting a predicate to False as part of an action's effect; but if that predicate is already false, then this causes an error. This change was intended to allow that behavior, not to allow non-existent predicates.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, yeah you're totally right, this would be a bug. It's a shame we didn't catch it earlier. Please remove this from the PR. I'll open a separate PR fixing this, and adding a test so the issue doesn't arise again.


def does_condition_hold(self, condition: Condition[Object]) -> bool:
"""Check if the given grounded condition holds in the state."""
Expand Down