-
Notifications
You must be signed in to change notification settings - Fork 4k
Fixed GraphPlan #1315
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
Open
carwyn987
wants to merge
20
commits into
aimacode:master
Choose a base branch
from
carwyn987:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fixed GraphPlan #1315
+1,180
−454
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…yet to pinpoint it.
…opriate level off condition that accounts for mutex changes. This version of graph plan works alright ... no plans tested fail, but the planner explosion, no_plan, double_tennis, air_cargo tests take too long for me to wait (i.e. they may fail).
… by a state and consecutive next action level. Our is our action mutexes, and our state mutex is computed by our previous state. So, when we first enter a level, we have our state mutexes (so we can check for goal state), then we nullify them and compute our action mutexes, and then we compute the next levels state mutexes based on our current action mutexes and next state. There is still at least one issue.
…ut tests seem to pass??? Not sure if actually correct or my drowsiness is making it look like greeon on my screen
…set of possible action tuples for each goal (but implemented efficiently with DFS recursive early-exit search
…ring_and_compute_prop_mutexes_on_expand_graph Experimental/revert ordering and compute prop mutexes on expand graph
…alence between prior layers. Furthermore, my mutex handling of overwriting with state mutexes causes this to fail anyway
…ion. Made previous tests coherent with updates
Not sure how to add reviewers, so I'll
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR Fixed GraphPlan algorithm. Changes were made to mutex calculation, representation of state vs actions, linearization. A test suite was added to comprehensively test the algorithm, which passes. These updates do not break any of the other tests.
Notable Changes / Additions
check_leveloff
FunctionThe function used to compute whether the search has leveled off (i.e. stagnated) only checked that the current and prior state were equivalent, but failed to check the state mutexes for equivalency - a necessary condition.
No Interference Computation
In the original implementation, Interference (one action deletes the precondition of another) seems to be ignored in our mutex calculations.
Explicit state and action mutexes
In the original implementation, state and action layers, as well as state (proposition) and action mutexes were jumbled together, with no differentiation.
Level compute/expand issue
In the original implementation, mutexes were computed after expanding a level and performing goal tests. Therefore, the goal test would not be able to test the goals until one extra layer had been expanded.
expand_graph
method to expand a layer, and populatestate_mutexes
before the goal test.Expanded GraphPlan tests
Currently tests are more focused on the environments than the algorithm. I've added a set of tests for GraphPlan to ensure functionality.
Fixes to the double tennis environment
Original env used
a
,b
in goals rather thanA
,B
as defined in initial.-Lowercase variables (e.g.
a
andb
) can be satisfied by any other variable, so the original test did pass with this formulation, but it likely was not the intention of the original authors.LeftNet
because it doesn't show up in initial or domain, only goal. It seems like all objects must exist within either the initial state or domain for the algorithm to search them.Potential Future Issues Identified
Extract_Solution Issue
extract_solution
is a method with the purpose of expanding the graph backwards from the goal state to the start state, identifying sub-goals for each layer along the way, and selecting a set of actions that fulfill this layers sub-goals, proceeding to the prior layer with the preconditions of those actions as new sub-goals. However, the authors wrote this implementation withitertools.product(*actions)
(Cartesian product), which chooses combinations of specifically one action that satisfies each goal. I'm not sure that this is theoretically guaranteed to identify a solution if one exists. However, the alternative of computing the cartesian product on all sets of actions that satisfy the goals (not just one action for each goal) is intractable with respect to compute resources.Memoization Improvement
In
extract_solution
, we use memoization to avoid searching for solutions to sub-problems that have already been proven unsolvable. However, we use an equality check, and do not consider solutions that are a superset of this subproblem. Adding this check could improve performance.Screenshots of tests:
Note - the two failing tests are not related to this PR or GraphPlan. These are not caused to fail from these modifications, they fail for me on a bare-bones clone. This proves to show that all relevant existing tests pass (i.e.
tests/test_planning.py
), are not damaged by these updates, and the new GraphPlan test filetests/test_graphplan.py
passes.