MAINT: Add documentation and descriptive variable names in search.py #1142
Conversation
search.py
Outdated
| for c in problem.actions(n): | ||
| if c in open_dir or c in closed_dir: | ||
| if g_dir[c] <= problem.path_cost(g_dir[n], n, None, c): | ||
| for child in problem.actions(n): |
There was a problem hiding this comment.
I am very confused here! Why is the child present in problem.actions(n) instead of n.expand(problem)?
Why does problem.actions(n) return the next states instead of actions possible in the state n?
There was a problem hiding this comment.
Inspecting further, it turns out that bidirectional search only supports GraphProblem type problems. I think we can add support for all the problem types by storing the nodes instead of states in open/closed list for both directions and calling n.expand(problem) on it to return the next states (gh-1144). Any other ideas?
There was a problem hiding this comment.
problem.actions(n) does return the actions possible in n, but since this is a graph problem, the actions possible are the next states. The function simply returns the neighbouring nodes, which are both the next states and actions possible. I believe though that you are right, if we want to support other types of problems, we need to switch to n.expand(problem) which is more general.
If you want, you can take this up, making bidirectional search general. That sounds like a great contribution!
There was a problem hiding this comment.
Yup, will fix that!
search.py
Outdated
| for c in problem.actions(n): | ||
| if c in open_dir or c in closed_dir: | ||
| if g_dir[c] <= problem.path_cost(g_dir[n], n, None, c): | ||
| for child in problem.actions(n): |
There was a problem hiding this comment.
I personally feel like c is descriptive enough, since in a lot of code c is used to denote the child.
| m, m_f = np.inf, np.inf | ||
| # pr_min_f isn't forward pr_min instead it's the f-value | ||
| # of node with priority pr_min. | ||
| pr_min, pr_min_f = np.inf, np.inf |
There was a problem hiding this comment.
I agree, the new variable names are indeed more descriptive, nice catch!
search.py
Outdated
|
|
||
| def find_key(pr_min, open_dir, g): | ||
| """Finds key in open_dir with value equal to pr_min | ||
| and minimum g value.""" | ||
| m = np.inf | ||
| gmin = np.inf |
There was a problem hiding this comment.
Here again I think that m is fine, since in what this does is a simple min search. In such code, with only one min, just writing m should be enough.
Add:
__hash__method ofNodeclass.