-
Notifications
You must be signed in to change notification settings - Fork 21
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
Issue 87 #90
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
==========================================
- Coverage 90.78% 90.77% -0.02%
==========================================
Files 24 24
Lines 1389 1398 +9
Branches 245 247 +2
==========================================
+ Hits 1261 1269 +8
Misses 93 93
- Partials 35 36 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor question...
pddl/requirements.py
Outdated
Requirements.NEG_PRECONDITION, | ||
Requirements.DIS_PRECONDITION, | ||
Requirements.EQUALITY, | ||
Requirements.QUANTIFIED_PRECONDITION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess quantified preconditions covers both existential and universal, but under that path, wouldn't STRIPS cover all those here?
Line 42 in f54e187
Requirements.TYPING, |
Actually, I'm a bit confused as to why those are all included in STRIPS...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, quantified preconditions covers both universal and existential.
Thanks for pointing out the issue. I think it all depends on what reference we are following and what we want to achieve. At this moment, STRIPS covers some of the other requirements but shouldn't (based on the original version and the BNF). I don't remember why we put such additional requirements under STRIPS, but most probably to parse some domains that are not fully compliant with the original grammar... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably flag it as an issue for the PDDL, and keep the pddl
lib pure and true to the BNF. Want to tackle the slight rewrite of the STRIPS set in this PR, or open a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree! I can do that here..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
Fixes
This PR solves the issue #87
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply.