Skip to content
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

Fixes for problem_generator.py #112

Merged
merged 11 commits into from
Dec 27, 2023
Merged

Conversation

trey0
Copy link
Contributor

@trey0 trey0 commented Dec 19, 2023

This is a pretty big code change that amounts to a pretty minor tweak to the planning model semantics but with some nice side benefits.

Main changes:

  • Previously, problem_generator.py output a PDDL problem instance in the format accepted by PDDL planners. However, when using PlanSys2, the PlanSys2 problem expert is in charge of generating the problem instance based on the knowledge incrementally built up by a sequence of PlanSys2 API calls. With this update, problem_generator.py is now compatible with PlanSys2. When run with --terminal, it generates a sequence of commands for the PlanSys2 terminal that will trigger the right API calls to the domain expert that should cause it to output an equivalent PDDL problem instance.
  • In the planner model and supporting scripts, we removed the run-number argument from stereo and panorama actions. We now think it's better to track run number using a counter outside of PlanSys2 so that the numbering state can persist across a PlanSys2 restart. A side benefit is a surprise ~60% reduction in planner run-time.
  • Another minor tweak is that we no longer specify num_orders in the config file, but rather we calculate it automatically based on the maximum order number used in the goals. This shrunk num_orders from 10 to 5 and further reduced planner run-time ~50%. (We might end up padding num_orders a bit again if we decide we need more flexibility to add new goals on the fly.)
  • Previously, problem_generator.py hard-coded the config file split between the "static" and "dynamic" configs. We've retained that split in the example inputs, but the script can now accept an arbitrary list of config files as long as they together contain all the necessary fields.
  • The scripts previously could be run for testing with their default arguments (which pointed to valid example files), but that got broken by a previous PR when everything was sorted into subfolders. Restored now.

Comment on lines 75 to 79
(completed-panorama
?robot - robot
?order - order
?location - location
?run-number - run-number
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plansys2 (specifically DomainReader) does not handle multi-line predicates as you'd expect. This has been reported upstream (PlanSys2/ros2_planning_system#286) but for now we need to axe the new-lines.

Comment on lines 90 to 95
(completed-stereo
?robot - robot
?order - order
?base ?bound - location
?run-number - run-number
)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plansys2 (specifically DomainReader) does not handle multi-line predicates as you'd expect. This has been reported upstream (PlanSys2/ros2_planning_system#286) but for now we need to axe the new-lines. (Github won't let me create a suggested change because the range encompasses a deleted line? That seems like a major limitation)

@Bckempa Bckempa mentioned this pull request Dec 21, 2023
Comment on lines 100 to 104
(completed-let-other-robot-reach
?robot - robot
?order - order
?loc - location
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as the other comments, but this time it lets me use the suggestion feature. Just make sure to apply to all three multi-line predicates:

Suggested change
(completed-let-other-robot-reach
?robot - robot
?order - order
?loc - location
)
(completed-let-other-robot-reach ?robot - robot ?order - order ?loc - location )

@Bckempa
Copy link
Contributor

Bckempa commented Dec 27, 2023

Discussed this with @bcoltin and we're going to merge this to keep testing unblocked

@Bckempa Bckempa merged commit a378d01 into nasa:survey_manager Dec 27, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants