-
Notifications
You must be signed in to change notification settings - Fork 145
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 panic error at shortest paths #1134
Conversation
Pull Request Test Coverage Report for Build 8207084623Details
💛 - Coveralls |
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.
Thanks for contributing this! The tests LGTM and so do the error messages. I did propose a simpler method to check if a node is present, though.
Also, you would mind expanding the fix to the Bellman-Ford and A* implementations? They are conceptually similar to Dijkstra's and were not included in the bug report. But they also panic now I believe
src/shortest_path/mod.rs
Outdated
if !graph | ||
.node_indices() | ||
.nodes | ||
.iter() | ||
.any(|&node| node == source) |
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.
I think we can simplify this to:
if !graph | |
.node_indices() | |
.nodes | |
.iter() | |
.any(|&node| node == source) | |
if !graph.graph.contains_node(start) |
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.
Note that you will need to move the if statement after the variable is defined
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.
Thanks, i really appreciate it the feedback, i gonna make the changes
src/shortest_path/mod.rs
Outdated
if !graph | ||
.node_indices() | ||
.nodes | ||
.iter() | ||
.any(|&index| index == node) | ||
{ | ||
return Err(PyIndexError::new_err(format!( | ||
"Node source index \"{node}\" out of graph bound" | ||
))); | ||
} | ||
|
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.
Same
src/shortest_path/mod.rs
Outdated
if !graph | ||
.node_indices() | ||
.nodes | ||
.iter() | ||
.any(|&index| index == node) | ||
{ |
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.
Same
src/shortest_path/mod.rs
Outdated
if !graph | ||
.node_indices() | ||
.nodes | ||
.iter() | ||
.any(|&node| node == source) |
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.
Same
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.
Awesome. This is good to
* lint suggestion UP032 * Test of dijkstra raising IndexError * Test Bellman Ford and A* * fix: shortest path (cherry picked from commit 058b0c7)
In #1117 is presented the presence of an uncaught
pyo3.PanicException
when the index of the source node is no out of bound in the graph. With this naive approach i try fix this issue raising a more understandableIndexError
.