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

[ALGO] Kosaraju's Algorithm #141

Merged
merged 10 commits into from
Oct 12, 2023
Merged

Conversation

toadkarter
Copy link
Contributor

@toadkarter toadkarter commented Oct 8, 2023

Implements #115

☑ The algorithm is implemented
☑ The new function has a javadoc-style comment explaining the interface
☑ Appropriate tests are added under test/graaflib/algorithm/strongly_connected_components/kosarajus_test.cpp
☑ A test coverage of at least 95% is reached
☑ A documentation entry is added under docs/docs/algorithms under the appropriate category. Just adding a short description and the algorithm syntax here is fine. See the wiki on how to build the documentation locally
☑ The algorithm is added to the list of algorithms in README.md

I should flag here that I decided to use a simple recursive DFS - the one that was already in the library seemed (unless I am mistaken) more geared towards searching through edges as opposed to vertices. However, if the library implementation of DFS is required please do let me know and I can work it into the existing code.

@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
...gorithm/strongly_connected_components/kosaraju.tpp 100.00% <100.00%> (ø)
...algorithm/strongly_connected_components/tarjan.tpp 100.00% <100.00%> (ø)
...hm/strongly_connected_components/kosaraju_test.cpp 100.00% <100.00%> (ø)
...ithm/strongly_connected_components/tarjan_test.cpp 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@toadkarter toadkarter marked this pull request as ready for review October 9, 2023 20:14
@toadkarter toadkarter changed the title [ALGO] Koraraju's Algorithm [ALGO] Kosaraju's Algorithm Oct 9, 2023
@bobluppes bobluppes self-requested a review October 10, 2023 06:00
@bobluppes bobluppes added the enhancement New feature label Oct 10, 2023
Copy link
Owner

@bobluppes bobluppes left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!
Looks quite good already, was just thinking about a possible issue regarding the order in which we add vertices to the set in the first DFS.

@toadkarter
Copy link
Contributor Author

Thanks for working on this! Looks quite good already, was just thinking about a possible issue regarding the order in which we add vertices to the set in the first DFS.

Thanks very much for the review! All of these suggestions look great and I will try to implement them when back from work today.

@bobluppes
Copy link
Owner

Thanks for working on this! Looks quite good already, was just thinking about a possible issue regarding the order in which we add vertices to the set in the first DFS.

Thanks very much for the review! All of these suggestions look great and I will try to implement them when back from work today.

Thanks a lot and no rush 👍🏻

@toadkarter toadkarter requested a review from bobluppes October 11, 2023 20:56
Copy link
Owner

@bobluppes bobluppes left a comment

Choose a reason for hiding this comment

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

Looks excellent 🎉

Comment on lines +8 to +9

using sccs_t = std::vector<std::vector<vertex_id_t>>;
Copy link
Owner

Choose a reason for hiding this comment

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

👍🏻 very nice that we now have this. Also thanks for updating Tarjan!

Comment on lines +137 to +141
TEST(KosarajuTest, DisconnectedGraphScenario) {
// GIVEN
const auto [graph, vertex_ids] =
utils::scenarios::create_disconnected_graph_scenario<
directed_graph<int, int>>();
Copy link
Owner

Choose a reason for hiding this comment

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

Also nice that we are now using these scenario's in the tests! I would like to start using them more throughout the codebase

@bobluppes bobluppes merged commit a1248a8 into bobluppes:main Oct 12, 2023
8 checks passed
@toadkarter toadkarter deleted the kosaraju-algorithm branch October 12, 2023 20:17
@toadkarter toadkarter restored the kosaraju-algorithm branch October 12, 2023 20:17
@toadkarter toadkarter deleted the kosaraju-algorithm branch October 12, 2023 20:17
@toadkarter toadkarter restored the kosaraju-algorithm branch October 12, 2023 20:17
@toadkarter toadkarter deleted the kosaraju-algorithm branch October 12, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants