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.
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
two sided dijkstra #268
base: master
Are you sure you want to change the base?
two sided dijkstra #268
Changes from 7 commits
9b43737
b37aefd
779b167
a2d65d4
d28db69
515091c
e3bede8
20b5bb8
2abd6b7
7504b6d
806593d
0de4e92
cb9eebe
e9aecef
40bb78a
22670f3
3b2d21a
313c464
08d696a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
All that white space may not be necessary
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.
This relax function clarifies things, would it be hard to add it to the original
dijkstra
too?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 was hoping you would notice =p I'll make the changes
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 have a new version of the
relax
function that should work for both thedijkstra_shortest_paths
andbidijkstra_shortest_path
but it is a bit clunky. I am wondering; why the need for theparents
andpreds
structures ? wouldn't the list of predecessors be enough ?A cosmetic note: should we rename the function the
bidijkstra_shortest_paths
if it should be able to deal with multiple paths ?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.
What if there are several paths?
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'm not sure how to deal with such situations, as far as I can tell multiple paths with the same cost can be detected if the condition
mu == ls + lt
is reached. I'll look into itThere 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.
Given that
bidijkstra_shortest_path
only returnssrc
-dst
-paths it doesn't make much sense to keep a full list of predecessors, does it ? I am not sure what the most elegant way to deal with these situations, especially when trying to share code withdijkstra_shortest_paths
.I've also noticed that there doesn't seem to be a straightforward way to retrieve multiple paths for a single
(src,dst)
pair. As far a I can tell theenumerate_paths
routine returns a single path per destination. Should this be addressed at some point ?