search

pucrs-automated-planning / pddl-parser

:snake: Classical Planning in Python
GNU General Public License v3.0
84 stars 23 forks source link

Added new features for :types. Faster planner data structures for comparisons. #4

Closed bcorfman closed 3 years ago

bcorfman commented 3 years ago

FIX: Beefed up :types to include type hierarchy. FEATURE: Changed preconditions, goals and visited data structures from lists to sets to speed comparisons. FEATURE: Moved Action's str method to repr and created a new str method instead to print plans in a more pleasing way. FEATURE: Updated test_PDDL.py to add tests for new :types features. FIX: Some general PEP8 code cleanup, although still incomplete.

Maumagnaguagno commented 3 years ago

You changed not only some attributes to frozensets (which is something I was delaying as it certainly breaks a lot of code), but also the class names. The reason most attributes are set in parse_domain/problem is to reset them to initial values, and avoid collision between multiple parsing runs, otherwise old values would be kept. If you want to add a feature don't make such big breaking changes at once, make small PRs that we can review and discuss. Class names and spacing are not up for discussion, they were made to either match HyperTensioN or my personal style.

EDIT: Note that Travis CI is failing.

bcorfman commented 3 years ago

Ah, sorry about the mess. I will make some edits to match up with your style and see if I can better match your expectations. I didn't realize you were supporting 2.7, for instance.

Maumagnaguagno commented 3 years ago

It is okay, just try to make small changes so that we can make a release before breaking compatibility with all other clones and forks. Compatibility with Python 2.7 can certainly be broken now. I would like to keep it working with legacy code, but you should not worry with that. I will modify Travis to allow 2.7 to fail and add a few more recent Pythons, but your code is also breaking 3.6 and 3.7, which means the version is not the only issue here.

bcorfman commented 3 years ago

got it. I'm still considering, but will probably close this PR and reopen a new one with just the :types changes first, since the other internal changes I made were more controversial. lol

Maumagnaguagno commented 3 years ago

Controversial is not the right word, you would panic too if someone tried to rename your classes :wink:. I fixed the comments that changed style and updated the CI config. Please open new PRs with your changes.

Maumagnaguagno commented 3 years ago

I will probably port some of the frozensets/tuples from our own modified version while you open the :types PR.