-
-
Notifications
You must be signed in to change notification settings - Fork 410
feat: add dijkstra with adjacency list and heap increasePriority #134
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
Conversation
Is there any reason not to convert the adjacency matrix representation into an adjacency list representation before executing Dijkstra? |
If the algorithm itself is doing the conversion, it increases space complexity from O(V) -> O(V + E). Outside of complexity, one could also argue for the benefits for an adjacency matrix in general. For example, adjacency lists that use linked lists may use up more heap space and have worse data locality, especially for dense graphs. I was not sure whether I should implement this with adjacency matrix or list, but the matrix implementation is easier. Going through existing thealgorithms implementations, I see a combination of both matrix and list implementations. Maybe its reasonable to have two alternative implementations? I think its reasonable for educational purposes. |
Now that I think about it, we could easily implement adjacency list with a 2-dimensional array. Each value is a 2-tuple with (destination_node_idx, weight). |
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.
Can you try to add a few (non-redundant) comments in the code?
I made changes for the small suggestions and pushed what I had. I tried an adjacency list implementation with this repo's MinHeap and found that its broken. I have fixes in #135. I would like to get that merged first before this PR. I'll be sure to add comments as suggested as well. |
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.
otherwise lgtm
I think I messed up to git stuff. Sorry for force push. I made changes for adjacency list, which now works since MinHeap was fixed. Added inline comments as suggested. |
Sorry, i think I am using priority queue incorrectly here. Im pushing the node index, but it should be the distance to the node. Please hold off on review for now. |
Indeed. If you override the comparison function of the priority queue to compare the distances of the nodes at the indices, you can keep working with indices. I believe this is a cleaner and more efficient solution than storing node - distance pairs. |
Add dijkstra shortest path algorithm for adjacency matrix.
We can get lower bounds on a graph with adjacency list representation using min/Fibonacci heap, but I think this solution is the lowest bound we can get for an adjacency matrix representation.