Skip to content
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

feat: add dijkstra with adjacency list and heap increasePriority #134

Merged
merged 4 commits into from
Jul 3, 2023

Conversation

caojoshua
Copy link
Contributor

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.

@appgurueu
Copy link
Contributor

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.

Is there any reason not to convert the adjacency matrix representation into an adjacency list representation before executing Dijkstra?

@caojoshua
Copy link
Contributor Author

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.

@caojoshua
Copy link
Contributor Author

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).

Copy link
Contributor

@appgurueu appgurueu left a 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?

graph/dijkstra.ts Outdated Show resolved Hide resolved
@caojoshua
Copy link
Contributor Author

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.

graph/dijkstra.ts Outdated Show resolved Hide resolved
appgurueu
appgurueu previously approved these changes Jun 11, 2023
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise lgtm

@caojoshua caojoshua changed the title feat: add dijkstra with adjacency matrix feat: add dijkstra with adjacency list Jun 12, 2023
@caojoshua
Copy link
Contributor Author

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.

@caojoshua
Copy link
Contributor Author

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.

@appgurueu
Copy link
Contributor

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.

@caojoshua caojoshua changed the title feat: add dijkstra with adjacency list feat: add dijkstra with adjacency list and heap increasePriority Jul 2, 2023
@raklaptudirm raklaptudirm merged commit 8deeca2 into TheAlgorithms:master Jul 3, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants