Fix/type hierarchy #73

francescofuggitti · 2023-06-01T16:04:18Z

Fixes

Fix typing parsing. Changed structure from list to dict. This PR addresses issue #70. As mentioned in #70, we only allow the either keyword in the parameters spec.

This PR also fixes #76, #77 and #78.

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

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

Put an x in the boxes that apply.

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

codecov-commenter · 2023-06-01T16:28:53Z

Codecov Report

Merging #73 (c649298) into main (ad68c05) will increase coverage by 1.09%.
The diff coverage is 93.78%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   88.30%   89.40%   +1.09%     
==========================================
  Files          20       22       +2     
  Lines        1052     1170     +118     
  Branches      164      191      +27     
==========================================
+ Hits          929     1046     +117     
+ Misses         95       94       -1     
- Partials       28       30       +2

Flag	Coverage Δ
unittests	`89.40% <93.78%> (+1.09%)`	⬆️

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

Impacted Files	Coverage Δ
pddl/logic/terms.py	`90.00% <62.50%> (-5.56%)`	⬇️
pddl/_validation.py	`88.23% <88.23%> (ø)`
pddl/core.py	`92.26% <92.85%> (-0.16%)`	⬇️
pddl/parser/domain.py	`84.27% <98.11%> (+3.27%)`	⬆️
pddl/constants.py	`100.00% <100.00%> (ø)`
pddl/custom_types.py	`100.00% <100.00%> (ø)`
pddl/exceptions.py	`71.42% <100.00%> (+14.28%)`	⬆️
pddl/formatter.py	`98.36% <100.00%> (+0.28%)`	⬆️
pddl/helpers/base.py	`93.65% <100.00%> (-0.10%)`	⬇️
pddl/logic/helpers.py	`100.00% <100.00%> (ø)`
... and 1 more

... and 2 files with indirect coverage changes

haz

Some fairly minor things / questions for ya.

haz · 2023-06-01T20:04:09Z

pddl/custom_types.py

+        }
+        if isinstance(names.values(), list)
+        else {name(type_def): [] for type_def in names}
+    )


This block's a little inscrutable for me. Can you rewrite with some commentary as to what's going on here? Both the indexing of [0] and isinstance(names.values(), list) look a little sus ;).

When parsing (:types ..., the type structure has the following types Dict[Token, Set]. Now, every type can have a single parent, so the [0] is to extract that element. (Note here we do not allow (either ....

When parsing (:predicates ..., each parameter may have >1 types with either, so a list as dict value is needed here.

Finally, the [] is to allow domains with types without hierarchy.

haz · 2023-06-01T20:07:29Z

pddl/formatter.py

+def _print_types_with_parents(types: Dict):
+    result = ""
+    for t in sorted(types.keys()):
+        result += f"{t} - {types[t]}" if types[t] else f"{t}"


Would this be capturing the ancestors? If it's just the direct types, wouldn't it need to be space-separated? Maybe what's unclear here is the form of the types dict -- if it's mapping a type to it's direct parent (no either allowed), then maybe indicate that in a docstring or comment (the variable types can be ambiguous).

Good point. I will make the form of the types dict more clear in the docstring.

haz · 2023-06-01T20:08:24Z

pddl/parser/domain.py

@@ -86,9 +86,11 @@ def requirements(self, args):

    def types(self, args):
        """Parse the 'types' rule."""
-        if not bool({Requirements.TYPING} & self._extended_requirements):
+        if any(args[2].values()) and not bool(


args[2].values()??

While following the BNF, I noticed that the :typing requirement is needed only when a hierarchy is defined, i.e., when the type_sep (-) is used. This is actually a tiny detail, we can get rid of it or I may try to make it more readable.

haz · 2023-06-01T20:09:06Z

pddl/parser/domain.py

+                args[type_sep_index + 1][1:]
+                if type(args[type_sep_index + 1]) is list
+                else args[type_sep_index + 1]
+            )


Some magic indexing happening ;). May be necessary, but a comment for our future selves would help.

Aye :) Since the method captures both types with an ancestor (for the (types ... definition) and types of variables (here either is allowed), this snippet assigns either the whole list of ancestors or the single ancestor depending on what has been parsed.

marcofavorito · 2023-06-02T16:37:21Z

pddl/core.py

@@ -41,7 +41,7 @@ def __init__(
        self,
        name: namelike,
        requirements: Optional[Collection["Requirements"]] = None,
-        types: Optional[Dict[namelike, Collection[namelike]]] = None,
+        types: Optional[Dict[namelike, Optional[namelike]]] = None,


idea: we could have a higher-level data structure instead of raw dictionaries, that would make easier to ask queries, e.g. all the parents of a given type, and guarantee other invariants, e.g. absence of cycles in the inheritance graph.

@haz and I discussed that in #70 and we agreed on using a dictionary for the moment. However, as actually seen in this PR, to accommodate things specific parts of the code are becoming increasingly fuzzy. So, I guess your proposal will ultimately be the way to maintain readability.

The only check performed now is whether there are cycles in the type def hierarchy.

…ith_parents'

as stated in #73 (comment), this piece of code could be made a bit more readable. This commit tries to make it more readable by giving a name to each condition of parsing failure due to missing type requirement.

This is appropriate to guarantee canonicity of the string representation.

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.

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 · 2023-06-03T19:31:33Z

tests/fixtures/pddl_files/storage/domain.pddl

+
+(define (domain Storage-Propositional)
+(:requirements :typing)
+(:types hoist surface place - object


Commit 22643a2 changed this line:

- :types hoist surface place area - object + (:types hoist surface place - object

Is that a right change?

Reason: area already stated below under a type (surface) that is narrower than object.

Also add some changes to utility functions, preparatory for future changes.

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.

Since in previous commits we splitted parsing of names from parsing of variables, we can make more stricter assumptions on the input of the method. This allowed to simplify the code to a large extent. This introduced some code duplication with the typed_list_names method; but the author's opinion is that it is a good-enough solution for the state of the codebase, where things can be refactored and changed rapidly. Moreover, note that, differently from the typed_list_names, typed_list_variables has to handle multiple types of variables, like the 'either' construct.

…k_types_dictionary function

…es (partially fix #77)

…domain's available types (fix #77)

…in's available types (fix #77)

marcofavorito · 2023-06-05T13:52:34Z

In my opinion, the PR is in a good state. The code regarding consistency checks and parsing, if possible, can be improved in the future; in this PR, we can focus on the functional part. Also, the PR is solving different issues related to typing, which is good, but at some point, we should stop and postpone other changes for another PR.

francescofuggitti · 2023-06-05T14:49:25Z

Thank you! I agree; we can proceed to merge this PR and iterate later on to improve. @haz ?

haz · 2023-06-05T14:55:19Z

Looking now...

haz · 2023-06-05T15:20:41Z

Looking pretty awesome to me! Not sure I'm a fan of the type_=.. syntax, but I don't have anything better to suggest at the moment ;). Let's merge it up.

francescofuggitti added 5 commits May 31, 2023 10:57

fix typing requirement check

bbdb5e0

add storage domain

80bd089

fix parsing for parent types

705bc8e

update type structure to dictionary, fix formatting

d276278

update tests

d5bcee7

francescofuggitti requested review from haz and marcofavorito June 1, 2023 16:04

haz requested changes Jun 1, 2023

View reviewed changes

marcofavorito mentioned this pull request Jun 2, 2023

Type hierarchy 2 #75

Merged

6 tasks

francescofuggitti added 2 commits June 2, 2023 12:17

address PR comments.

bcf10c4

chore: apply black

9d195b0

marcofavorito reviewed Jun 2, 2023

View reviewed changes

test: add test on type hierarchy pasing (#70)

6800afe

francescofuggitti requested a review from haz June 2, 2023 17:57

marcofavorito added 2 commits June 3, 2023 16:34

feat: add 'find_cycle' function in helpers

a7f41a4

feat: add validation of types hierarchy

43fdbfc

The only check performed now is whether there are cycles in the type def hierarchy.

marcofavorito force-pushed the fix/type_hierarchy branch from 048f28d to 43fdbfc Compare June 3, 2023 14:41

marcofavorito added 5 commits June 3, 2023 16:56

fix: add docstring and arg typing for helper function '_print_types_w…

31877aa

…ith_parents'

fix: make types parsing more readable in domain parser

f64a119

as stated in #73 (comment), this piece of code could be made a bit more readable. This commit tries to make it more readable by giving a name to each condition of parsing failure due to missing type requirement.

fix: sort type tags in 'either' types in predicate formatting

0aabe7d

This is appropriate to guarantee canonicity of the string representation.

fix: typed_list_name parsing in domain.lark

a3af26d

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 reviewed Jun 3, 2023

View reviewed changes

marcofavorito added 3 commits June 3, 2023 22:42

fix: add duplication tests also for constants.

0ea297e

Also add some changes to utility functions, preparatory for future changes.

marcofavorito force-pushed the fix/type_hierarchy branch from 740f3d8 to fea7daf Compare June 3, 2023 21:16

marcofavorito added 2 commits June 4, 2023 00:07

fix: use PDDLValidationError rather than generic ValueError for _chec…

033893d

…k_types_dictionary function

feat: add validation of constant types against domain's available typ…

691390e

…es (partially fix #77)

marcofavorito force-pushed the fix/type_hierarchy branch from fa06450 to 691390e Compare June 3, 2023 22:07

marcofavorito added 2 commits June 4, 2023 23:23

feat: add validation of predicates' and actions' terms types against …

2cf276a

…domain's available types (fix #77)

feat: add validation of derivate predicates' terms types against doma…

f015fb4

…in's available types (fix #77)

marcofavorito force-pushed the fix/type_hierarchy branch from 4cf18a3 to f015fb4 Compare June 4, 2023 21:30

marcofavorito added 2 commits June 4, 2023 23:36

fix: add repr function to Action

f58a1be

fix: bug in typed list of variable, 'either' was not handled correctly

c649298

fix: detect if object is used as subtype (fix #78)

5cd8ab2

marcofavorito force-pushed the fix/type_hierarchy branch from 551f195 to 5cd8ab2 Compare June 5, 2023 14:19

haz approved these changes Jun 5, 2023

View reviewed changes

haz merged commit dcb71de into main Jun 5, 2023
10 checks passed

marcofavorito deleted the fix/type_hierarchy branch June 5, 2023 16:37

marcofavorito mentioned this pull request Jun 5, 2023

Not parsing the type hierarchy #70

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/type hierarchy #73

Fix/type hierarchy #73

francescofuggitti commented Jun 1, 2023 •

edited by marcofavorito

Loading

codecov-commenter commented Jun 1, 2023 •

edited

Loading

haz left a comment

haz Jun 1, 2023

francescofuggitti Jun 2, 2023

haz Jun 1, 2023

francescofuggitti Jun 2, 2023

haz Jun 1, 2023

francescofuggitti Jun 2, 2023

haz Jun 1, 2023

francescofuggitti Jun 2, 2023

marcofavorito Jun 2, 2023

francescofuggitti Jun 2, 2023 •

edited

Loading

marcofavorito Jun 3, 2023 •

edited

Loading

marcofavorito Jun 5, 2023 •

edited

Loading

marcofavorito commented Jun 5, 2023 •

edited

Loading

francescofuggitti commented Jun 5, 2023

haz commented Jun 5, 2023

haz commented Jun 5, 2023

Fix/type hierarchy #73

Fix/type hierarchy #73

Conversation

francescofuggitti commented Jun 1, 2023 • edited by marcofavorito Loading

Fixes

Types of changes

Checklist

codecov-commenter commented Jun 1, 2023 • edited Loading

Codecov Report

haz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francescofuggitti Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

marcofavorito Jun 3, 2023 • edited Loading

Choose a reason for hiding this comment

marcofavorito Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

marcofavorito commented Jun 5, 2023 • edited Loading

francescofuggitti commented Jun 5, 2023

haz commented Jun 5, 2023

haz commented Jun 5, 2023

francescofuggitti commented Jun 1, 2023 •

edited by marcofavorito

Loading

codecov-commenter commented Jun 1, 2023 •

edited

Loading

francescofuggitti Jun 2, 2023 •

edited

Loading

marcofavorito Jun 3, 2023 •

edited

Loading

marcofavorito Jun 5, 2023 •

edited

Loading

marcofavorito commented Jun 5, 2023 •

edited

Loading