Fixes and improvements in the parsing of typed lists

marcofavorito commented 1 year ago

Proposed changes

This PR brings several fixes and improvements in the parsing of typed lists (names/variables).

Fixes

n/a

Types of changes

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

[X] 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

Further comments

n/a

codecov-commenter commented 1 year ago

Codecov Report

Merging #82 (edcc295) into main (b25b3a4) will increase coverage by 0.62%. The diff coverage is 94.66%.

:exclamation: 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      #82      +/-   ##
==========================================
+ Coverage   89.67%   90.30%   +0.62%     
==========================================
  Files          22       24       +2     
  Lines        1182     1423     +241     
  Branches      193      247      +54     
==========================================
+ Hits         1060     1285     +225     
- Misses         92      102      +10     
- Partials       30       36       +6

Flag	Coverage Δ
unittests	`90.30% <94.66%> (+0.62%)`	:arrow_up:

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

Impacted Files	Coverage Δ
pddl/logic/base.py	`86.66% <75.00%> (-1.01%)`	:arrow_down:
pddl/core.py	`92.12% <87.71%> (-0.14%)`	:arrow_down:
pddl/requirements.py	`90.00% <90.00%> (ø)`
pddl/action.py	`93.02% <93.02%> (ø)`
pddl/formatter.py	`95.94% <93.75%> (-2.42%)`	:arrow_down:
pddl/_validation.py	`92.30% <94.59%> (+2.56%)`	:arrow_up:
pddl/parser/typed_list_parser.py	`94.87% <94.87%> (ø)`
pddl/custom_types.py	`100.00% <100.00%> (ø)`
pddl/helpers/base.py	`97.40% <100.00%> (+3.75%)`	:arrow_up:
pddl/logic/predicates.py	`94.56% <100.00%> (+0.44%)`	:arrow_up:
... and 4 more

... and 1 file with indirect coverage changes

ssardina commented 1 year ago

hi @francescofuggitti @marcofavorito , I was about to submit a PR to fix the constant section the domain, as they currently do not come with their types.

However, I just found this massive PR that seems to deal with related things and will change many things, so don't want to cause conflicts and so I am planning to wait until this is merge. Is it almost done?

I think the problem is in pddl.formatter:

    body += _sort_and_print_collection("(:constants ", domain.constants, ")\n")

That can be improved I think to do the same as objects in problems... I think.

(BTW, great work on this parser, it's very clean!)

marcofavorito commented 1 year ago

hi @francescofuggitti @marcofavorito , I was about to submit a PR to fix the constant section the domain, as they currently do not come with their types.

However, I just found this massive PR that seems to deal with related things and will change many things, so don't want to cause conflicts and so I am planning to wait until this is merge. Is it almost done?

I think the problem is in pddl.formatter:
    body += _sort_and_print_collection("(:constants ", domain.constants, ")\n")
That can be improved I think to do the same as objects in problems... I think.

(BTW, great work on this parser, it's very clean!)

Hi @ssardina , thank you for your appreciation... being useful to the community is the lymph for our work and motivation!

Given your message, we will finish the PR very very soon (~hours). We will keep you posted here :)

marcofavorito commented 1 year ago

@ssardina this commit edcc295 should solve your request. The formatting issue was planned for another PR, but it is even better that we've added it here.

@francescofuggitti, @haz, any concerns? Would you like to give a final review? I have other changes planned (integrate consistency checks, started here), but I think this PR is already too big; so I will continue on another branch/PR.

ssardina commented 1 year ago

@ssardina this commit edcc295 should solve your request. The formatting issue was planned for another PR, but it is even better that we've added it here.

Thanks, yes it seems to work well, with types listed for constants!

Thanks!

ssardina commented 1 year ago

Now @marcofavorito , this is related to types and this PR, but maybe you want to handled it later on another thread. It doesn't like a type having two parent types, like in this Storage case:

Is this part of PDDL constraints or we are over-checking here:

This is the error given in the main branch:

However, in this PR I get an even more primitive error complaining that area is duplicated:

I believe a parent type can indeed be a child type in PDDL and is used in many domains, right? Like here area:

marcofavorito commented 1 year ago

Thank you for your reply @ssardina . In this issue #70, there has been a discussion regarding whether we should support them or not. The discussion converged to NOT supporting types with multiple parent types (I think it is summarized by this comment, that cites the PDDL textbook: https://github.com/AI-Planning/pddl/issues/70#issuecomment-1564858353)

I would be very glad to know your opinion on the topic though!

marcofavorito commented 1 year ago

Regarding the problem on area, thank you, it will be fixed very soon!

marcofavorito commented 1 year ago

However, in this PR I get an even more primitive error complaining that area is duplicated: