DFKI-NI / mobipick_labs

Other
4 stars 1 forks source link

Add hierarchical domain to tables demo #34

Closed marcvinci closed 1 year ago

marcvinci commented 1 year ago

Problem: After adding multiple planning methods to enable replanning for the tables demo, the planning time increased significantly to around ~17 seconds. Without these methods it only takes ~0.6 seconds. These methods are necessary for the tables demo to continue if something goes wrong. Without them the planner does not know which actions to skip based on the current state of the environment. They can be seen here: https://github.com/DFKI-NI/mobipick_labs/blob/1b6be5e70ac61ffb66d7c8b940f0cbcca11d0ae9/tables_demo_planning/src/tables_demo_planning/hierarchical_domain.py#L364-L548

alexander-sung commented 1 year ago

I might give some feedback in random order while browsing through the changes.

Method get_type() is used a lot in hierarchical_domain.py but actually only for Task and Method. Have you considered storing the UP types in variables or, if the usage should be available independent of the HierarchicalDomain, providing "TaskAPI" / "MethodAPI" classes which accepts application domain parameters and maps them to their UP representations? This is not about runtime efficiency but code readability and probably mostly about comfort of use for other users.

alexander-sung commented 1 year ago

I'm surprised about the hierarchical planning taking so long. Isn't the idea that with the effort spent on declaring the hierarchy of actions, planning should be actually easier and thus faster? So, I'm not sure if I'm the right one to review the hierarchical part. Maybe Sebastian is needed here.

I don't see any problems with merging the pull request since you saw to it that the other demo versions remain unaffected but as of now I can't comment whether the hierarchical approach needs improvement.

alexander-sung commented 1 year ago

Please show me the circumstances of the "noop" methods. Maybe there is something we can do with the original methods to remedy it? I'm guessing, though, so might need a demonstration first.

alexander-sung commented 1 year ago

One more though regarding the merge conflict: Please rebase, not merge from noetic. When resolving the conflict, make sure there is a leading space in the second line of text (currently missing in both conflicting files).