Create simple_paths_generator_with_score.rs #1284
Conversation
It uses Dijkstra Algo to generate all possible paths , by deleting one edge at a time, and gives the paths with their score.
|
|
|
@mtreinish can you review the PR and suggest how we move forward. |
…_with_score_multithreading.rs
Pull Request Test Coverage Report for Build 11030598879Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Thanks for your first contribution to rustworkx, I did a quick first pass through the code and left some high level inline comments. The biggest things missing right now is that this code isn't actually included in the library, the module isn't included anywhere that gets linked in from the root library. This means while the source file is in tree nothing is building it as part of rustworkx-core. Besides not including the source as part of the library this is preventing things like the formatter and linter from actually running on it. I think you should move your new file out of rustworkx-core/src/generators/ and into rustworkx-core/src/shortest_path, then you will need to modify https://github.com/Qiskit/rustworkx/blob/main/rustworkx-core/src/shortest_path/mod.rs to include your new module and re-export the public interface like is done for the other shortest path functions. Once that is done then the code will be included in the library at build time.
Following that I expect there will be lint and formatting things to fix as I observed a couple of places that needed to be fixed. The other thing is I left a comment inline about tests we should be sure we're exercising the code as part of the PR to make sure we're adequately testing everything. I would suggest taking a look at the project's contributing guide: https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md which covers these aspects in a bit more detail.
I haven't gone through the bulk of the logic of the new function yet, I'll review the algorithm and the function in more depth along with a deeper code review when these higher level issues are fixed. There is also the question of introducing a python interface too, but we can discuss that after we have a functional rustworkx-core function.
| } | ||
|
|
||
| // This is mutation of petgraph dijkastra to get full path between source to target | ||
| fn dijkstra<G, F, K>( |
There was a problem hiding this comment.
How is this different from the already existing https://github.com/Qiskit/rustworkx/blob/main/rustworkx-core/src/shortest_path/dijkstra.rs#L100 function?
There was a problem hiding this comment.
I have not review this specific Dijkastra, but petgraph dijkastra returns only score of each node between source & target. but how to find the path from source -> target is not clear from that score. thats why i have updated the code to return the previous node as, that it followed to reach to the destination.
like - if the path for lowest score was A->B-->C->D, it will return the score, also Node (C), which is the previous node, similarly for C , it will return score & B as previous node, and for B it will return A as previous node, which we can use to reach till A from D.
Let me know if I am missing something on existing Dijkastra functioning.
| loop { | ||
| let pre_node = path.get(node).expect("Error"); | ||
| paths.push(*pre_node); | ||
| if *pre_node == s.source { | ||
| break; | ||
| } | ||
| node = pre_node; | ||
| } |
There was a problem hiding this comment.
In general it'd be better to rework this as a while loop so the exit conditions are a bit more clear.
There was a problem hiding this comment.
In general it'd be better to rework this as a while loop so the exit conditions are a bit more clear.
it actually back tracks the path till source, if the next node is source, then it exits.
There was a problem hiding this comment.
see the current status, even though its same with comment one will get to know the exit state.
|
Please review the PR -
|
It uses Dijkstra Algo to generate all possible paths , by deleting one edge at a time, and gives the paths with their score.
Also provides a alternative for existing k-shortest-paths function, as that fails on some test cases.
Related to #671