-
-
Notifications
You must be signed in to change notification settings - Fork 46.6k
Fix greedy_best_first #8775
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
Fix greedy_best_first #8775
Conversation
graphs/greedy_best_first.py
Outdated
@@ -136,22 +153,21 @@ def get_successors(self, parent: Node) -> list[Node]: | |||
pos_x = parent.pos_x + action[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could get the successors using a list comprehension:
successors = [
Node(
(pos_x := parent.pos_x + action[1]),
(pos_y := parent.pos_y + action[0]),
self.target.pos_x,
self.target.pos_y,
parent.g_cost + 1,
parent,
)
for action in delta
if (
0 <= pos_x < len(self.grid[0])
and 0 <= pos_y < len(self.grid)
and self.grid[pos_y][pos_x] == 0
)
]
but ultimately I could go either way since this isn't necessarily more readable than what you already have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tianyizheng02
thank for your comment. i tried but i got this failed.
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1
graphs/greedy_best_first.py:153:18: F841 Local variable `pos_x` is assigned to but never used
|
151 | return [
152 | Node(
153 | (pos_x := parent.pos_x + action[1]),
| ^^^^^ F841
154 | (pos_y := parent.pos_y + action[0]),
155 | self.target.pos_x,
|
= help: Remove assignment to unused variable `pos_x`
graphs/greedy_best_first.py:154:18: F841 Local variable `pos_y` is assigned to but never used
|
152 | Node(
153 | (pos_x := parent.pos_x + action[1]),
154 | (pos_y := parent.pos_y + action[0]),
| ^^^^^ F841
155 | self.target.pos_x,
156 | self.target.pos_y,
|
= help: Remove assignment to unused variable `pos_y`
Found 2 errors.
black....................................................................Passed
codespell................................................................Passed
pyproject-fmt........................................(no files to check)Skipped
Validate filenames.......................................................Passed
Validate pyproject.toml..............................(no files to check)Skipped
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1
graphs/greedy_best_first.py:162: error: Name "pos_x" is used before definition [used-before-def]
graphs/greedy_best_first.py:163: error: Name "pos_y" is used before definition [used-before-def]
graphs/greedy_best_first.py:164: error: Name "pos_y" is used before definition [used-before-def]
graphs/greedy_best_first.py:164: error: Name "pos_x" is used before definition [used-before-def]
Found 4 errors in 1 file (checked 1 source file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think this is the correct syntax?
successors = [
Node(
pos_x,
pos_y,
self.target.pos_x,
self.target.pos_y,
parent.g_cost + 1,
parent,
)
for action in delta
if (
0 <= (pos_x := parent.pos_x + action[1]) < len(self.grid[0])
and 0 <= (pos_y := parent.pos_y + action[0]) < len(self.grid)
and self.grid[pos_y][pos_x] == 0
)
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tianyizheng02
Thanks, I applied list comprehension.
- node in self.open_nodes is always better node TheAlgorithms#8770
for more information, see https://pre-commit.ci
4ba71ab
to
c63a974
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor tweaks, but other than that LGTM
* fix: typo TheAlgorithms#8770 * refactor: delete unnecessary continue * add test grids * fix: add \_\_eq\_\_ in Node class TheAlgorithms#8770 * fix: delete unnecessary code - node in self.open_nodes is always better node TheAlgorithms#8770 * fix: docstring * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: docstring max length * refactor: get the successors using a list comprehension * Apply suggestions from code review --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Tianyi Zheng <[email protected]>
Describe your change:
fixes: #8770
Checklist:
Fixes: #{$ISSUE_NO}
.