Fix: Plan instantiation not respecting types of action parameters

[RichardGubijev]

Proposed changes

Type the parameters used in instantiating actions in Plan.instantiate() using the existing type_tags variables of the to-be instantiated actions.

Demonstration:

from pddl import parse_plan, parse_domain, parse_problem
from pddl.logic.predicates import Predicate
from pddl.logic.base import Not, And

problem = parse_problem("tests/fixtures/pddl_files/barman-sequential-optimal/p01.pddl")
domain = parse_domain("tests/fixtures/pddl_files/barman-sequential-optimal/domain.pddl")
plan = parse_plan("tests/fixtures/pddl_files/barman-sequential-optimal/p01.plan")

problem.domain = domain
actions = plan.instantiate(domain)
    
print("preconditions:")
for action in actions:
    for effect in action.precondition.operands if isinstance(action.precondition, And) else [action.precondition]:
        pred = effect._arg if isinstance(effect, Not) else effect
        print_terms = []
        for term in pred.terms:
            print_terms.append(f"{term.name}: {term.type_tag}")
        print(str(pred), *print_terms)

print("\n\neffects:")
for action in actions:
    for effect in action.effect.operands if isinstance(action.effect, And) else [action.effect]:
        pred = effect._arg if isinstance(effect, Not) else effect
        if not isinstance(pred, Predicate): continue
        print_terms = []
        for term in pred.terms:
            print_terms.append(f"{term.name}: {term.type_tag}")
        print(str(pred), *print_terms)

Without the fix (truncated):

preconditions:
(ontable shaker1) shaker1: None
(handempty left) left: None
(ontable shot4) shot4: None
(handempty right) right: None
(holding left shaker1) left: None shaker1: None
...


effects:
(ontable shaker1) shaker1: None
(handempty left) left: None
(holding left shaker1) left: None shaker1: None
(ontable shot4) shot4: None
(handempty right) right: None
(holding right shot4) right: None shot4: None
(holding left shaker1) left: None shaker1: None
...

With the fix (truncated):

preconditions:
(ontable shaker1) shaker1: container
(handempty left) left: hand
(ontable shot4) shot4: container
(handempty right) right: hand
(holding left shaker1) left: hand shaker1: container
(holding right shot4) right: hand shot4: shot
...


effects:
(ontable shaker1) shaker1: container
(handempty left) left: hand
(holding left shaker1) left: hand shaker1: container
(ontable shot4) shot4: container
(handempty right) right: hand
...

Fixes

When you instantiate a plan, the actions are instantiated with terms without type_tags, even if the variables in the actions do.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have read the CONTRIBUTING doc
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works

Further comments

I'm looking for feedback for if this is something the Plan.instantiate() should do, if it's better to do it somewhere else, or if the terms are meant to be untyped.

[RichardGubijev]

For context on why this might be needed, I need the terms in the effects to be typed and to achieve that I've done it by looking up the items in the Problem.objects

def type_action_list_terms(pddl_problem: Problem, actions: list[Action]):
    new_actions = []
    for action in actions:
        new_actions.append(Action(action.name, action.parameters, type_formula_terms(pddl_problem, action.precondition), type_formula_terms(pddl_problem, action.effect)))
    return new_actions

def type_formula_terms(problem: Problem, formula: Formula):
    if isinstance(formula, And):
        return And(*[type_formula_terms(problem, pred) for pred in formula._operands])
    if isinstance(formula, Not):
        return Not(type_predicate_terms(problem, formula.argument))
    else:
        return type_predicate_terms(problem, formula)

def type_predicate_terms(pddl_problem: Problem, predicate: Predicate):
    have_to_type = False
    for term in predicate.terms:
        if len(term.type_tags) <= 0:
            have_to_type = True
            break
    if have_to_type:
        new_terms = []
        for term in predicate.terms:
            if len(term.type_tags) <= 0:
                new_terms.append(Constant(term.name, get_type_tag(pddl_problem, term.name)))
            else:
                new_terms.append(term)
        return Predicate(predicate.name, *new_terms)
    return predicate

def get_type_tag(pddl_problem: Problem, name: str):
    for obj in pddl_problem.objects:
        if name == obj.name:
            return obj.type_tag
    else:
        return None

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.83%. Comparing base (8da9fec) to head (df51c37).
⚠️ Report is 79 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
+ Coverage   88.12%   89.83%   +1.70%     
==========================================
  Files          24       27       +3     
  Lines        1752     1997     +245     
  Branches      233      216      -17     
==========================================
+ Hits         1544     1794     +250     
+ Misses        148      140       -8     
- Partials       60       63       +3     

Flag	Coverage Δ
unittests	89.83% <100.00%> (+1.70%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
pddl/core.py	92.71% <100.00%> (+0.54%)	⬆️
pddl/helpers/base.py	97.43% <100.00%> (-0.37%)	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[haz]

Thanks, @RichardGubijev ! I think this change makes sense. The instantiation of plans is fairly new functionality, so it makes sense that we're missing some things. Want to throw in some tests that fail before your fix, but pass now? After that, feel free to mark it as "Ready for review"

[RichardGubijev]

I modified a test to check if the instantiated action's parameter terms are typed, it's not the most robust possible test, but I am not sure on how to test this feature/fix.

Let me know what y'all think and if you want more robust test coverage I'd appreciate some pointers/help with it!