Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not parsing the type hierarchy #70

Closed
francescofuggitti opened this issue May 26, 2023 · 9 comments
Closed

Not parsing the type hierarchy #70

francescofuggitti opened this issue May 26, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@francescofuggitti
Copy link
Collaborator

Subject of the issue

The type hierarchy is not preserved when parsing a PDDL domain.

Steps to reproduce

To reproduce the error, just try to parse a domain with a defined type hierarchy, such as this domain.

Expected behaviour

The parsed domain types should be

(:types truck airplane - vehicle
        package vehicle - physobj
        airport location - place
        city place physobj - object)

Actual behaviour

The actual parsed types are:
(:types airplane airport city location package physobj place truck vehicle)

@francescofuggitti francescofuggitti added the bug Something isn't working label May 26, 2023
@haz
Copy link
Contributor

haz commented May 26, 2023

Self-contained code to test:

from pddl.formatter import domain_to_string
from pddl.parser.domain import DomainParser

DOM = """
(define (domain logistics)
  (:requirements :strips :typing) 
  (:types truck
          airplane - vehicle
          package
          vehicle - physobj
          airport
          location - place
          city
          place 
          physobj - object)
  
  (:predicates 	(in-city ?loc - place ?city - city)
		(at ?obj - physobj ?loc - place)
		(in ?pkg - package ?veh - vehicle))
  
(:action LOAD-TRUCK
   :parameters    (?pkg - package ?truck - truck ?loc - place)
   :precondition  (and (at ?truck ?loc) (at ?pkg ?loc))
   :effect        (and (not (at ?pkg ?loc)) (in ?pkg ?truck)))

)
"""
domain = DomainParser()(DOM)

print(f"\n\tTypes ({type(domain.types)}):\n\t  {domain.types}\n")

print(domain_to_string(domain))

I think the types should ultimately be a dictionary mapping a type to its parent type. Then objects should have all the types they belong to be accessible.

@francescofuggitti
Copy link
Collaborator Author

I also think that the mapping dictionary is the solution to go. However, I'm just wondering how we can cover the (afaik not so common) (either <primitive-type>+) case. I guess that covering this case would require a more structured approach, i.e., defining a Type class inheriting from a PrimitiveType base class. What do you think?

@haz
Copy link
Contributor

haz commented May 26, 2023

What are the semantics again? A type inherits from two others? If that's the case, then we can just have the dict map a type to a list of types (most often just one)

@francescofuggitti
Copy link
Collaborator Author

Apparently, a type inherits only from a single type. But there may be many of such parent types.

@haz
Copy link
Contributor

haz commented May 26, 2023

Hrmz...this isn't in the type def, though, right? Just the parameters of an action or predicate (i.e., typing an object). Across all the classical domains in the API, only storage has it used:

(:predicates (clear ?s - storearea)
             (in ?x - (either storearea crate) ?p - place)
             (available ?h - hoist)
             (lifting ?h - hoist ?c - crate)
             (at ?h - hoist ?a - area)
             (on ?c - crate ?s - storearea)
             (connected ?a1 ?a2 - area)
             (compatible ?c1 ?c2 - crate))

@francescofuggitti
Copy link
Collaborator Author

Although most of the examples are on predicates or actions, from the BNF grammar (here) it seems that it can be a type def as well. But you're right, it rarely (if not never) appears as type def.

<types-def>              ::= (:types <typed list (name)>)
[...]
<typed list (x)>         ::= x*
<typed list (x)>         ::= x+ - <type> <typed list(x)>
<primitive-type>         ::= <name>
<primitive-type>         ::= object
<type>                   ::= (either <primitive-type>+)
<type>                   ::= <primitive-type>

@haz
Copy link
Contributor

haz commented May 26, 2023

My vote is to ignore the BNF (it was always unofficial (this repo is what makes things official!)) and just allow it for the parameter specs. An excerpt from "the book" on the matter...

image

@francescofuggitti
Copy link
Collaborator Author

Yep, I agree. It should also be easier to implement.

@francescofuggitti francescofuggitti mentioned this issue Jun 1, 2023
6 tasks
@marcofavorito marcofavorito mentioned this issue Jun 2, 2023
6 tasks
marcofavorito added a commit that referenced this issue Jun 3, 2023
According to the discussion in #70, only variables will have disjunctive type specifications.

Constants have only one type. Moreover, types can only have one supertype, i.e. no multiple inheritance.
marcofavorito added a commit that referenced this issue Jun 3, 2023
This commit fixes issue #76 and makes the parsing of typed lists of names separated from the typed lists of variables.

Regarding the separation, since in the discussion in #70 and in a3af26d has been decided that "typed lists of names" should not have more than one parent (where 'parent' means a supertype for types, and a 'type' for constants), the parsing of typed list of names can be made separated from the parsing of other typed lists, e.g. the one of variables. This should simplify the code and make it more readable.

Regarding issue #76, this commit adds two types of duplicates checks:

1) a check over simple typed lists, that simply checks whether there are two occurrences of the same item (which in PDDL terms means either duplicated types or duplicated constants);
2) a check over (complex) typed lists (i.e. with typed hierarchy), where we check whether multiple occurrences of names occurred (e.g. "constant_1 - type_1 constant_1 - type_2").

Tests for both checks have been added.

Moreover, the 'storage' domain "IPC5 Domain: Storage Propositional" (path: tests/fixtures/pddl_files/storage/domain.pddl) had a duplication issue since the type of area was specified twice.
marcofavorito added a commit that referenced this issue Jun 3, 2023
As discussed in #70, and in particular in comment #70 (comment) and #70 (comment), it has been decided to not allow multiple types excepts for action parameters and variables.

This commit fixes the previous code regarding constants according to the above decision, namely, that constants should have at most one type specification.

This change had some implication across the codebase. E.g.:
- the initializer method of Constant should have a single optional type, rather than an optional collection of types
- the utility function 'constants' is changed accordingly with the initializer
- the problem parser is changed by making it to use DomainTransformer.typed_list_name, rather than DomainTransformer.typed_list_variables, to parse objects.
@marcofavorito
Copy link
Member

Closed by #73

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants