-
Notifications
You must be signed in to change notification settings - Fork 27
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
Support for numeric fluents and action costs #92
Conversation
…l-fluents # Conflicts: # pddl/core.py # pddl/formatter.py # pddl/helpers/base.py # pddl/parser/domain.py # pddl/parser/symbols.py # tests/test_formatter.py
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
- Coverage 90.77% 88.10% -2.67%
==========================================
Files 24 25 +1
Lines 1398 1791 +393
Branches 247 332 +85
==========================================
+ Hits 1269 1578 +309
- Misses 93 154 +61
- Partials 36 59 +23
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.
Thank you so much for your efforts on this @francescofuggitti ! It's an epic addition. Especially by the diff count ;)
I suspect I'm missing some things that will come to light when we release this to the wild, but I've just made a few (mostly minor) comments/questions for you to consider.
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.
This is a monumental work @francescofuggitti, thank you so much. The tests witness that the feature is working (regardless of minor issues that might pop up in the future). Thanks!! 🙏
0991d42
to
7eacf32
Compare
…ested duplicated elements (found by @marcofavorito, thanks!).
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 some open question on what's all resolved. Rest for @marcofavorito to sign off on.
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
Proposed changes
In this PR, we add support for numeric fluents and action costs. It is built on top of the work originally proposed in #59. However, some updates on the lark domain and problem grammars and data structures have been applied to support all features of numeric fluents.
This PR primarily addresses issues #38 and #89, but it also fixes the issues in #93 and #96.
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.