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

Add minimum spanning tree algorithm #22

Closed
wants to merge 3 commits into from

Conversation

Aliqyan-21
Copy link

minimum_spanning_tree.h

In this file, I have define a helper struct DisjointSet for managing disjoint sets. The minimum_spanning_tree function implements the Kruskal's algorithm for finding the minimum spanning tree. It utilizes the DisjointSet data structure and sorts the edges in ascending order based on their weights. The algorithm iterates over the sorted edges and adds an edge to the minimum spanning tree if it does not form a cycle.

minimum_spanning_tree.tpp

Since the entire implementation of the minimum_spanning_tree function is already coded in the header file, there is no additional implementation required in the "minimum_spanning_tree.tpp" file. The code in the header file is already complete and self-contained.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi there! Thank you for creating your first pull-request on the Graaf library :)

@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a984e36) 100.00% compared to head (ede23c9) 100.00%.

❗ Current head ede23c9 differs from pull request most recent head 26fbf69. Consider uploading reports for the commit 26fbf69 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #22   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          411       411           
=========================================
  Hits           411       411           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Hi @Aliqyan-21, thanks for working on this 🚀 I will take a closer look at the PR this weekend.

In the mean time, you could take a look at adding unit tests for the new logic (you can add it under test/graaflib/algorithm/minimum_spanning_tree_test.cpp).

Currently the CI also fails on the formatting of the project, you can find more info on how to format your changes locally on this wiki page.

@bobluppes bobluppes mentioned this pull request May 28, 2023
2 tasks
@Aliqyan-21
Copy link
Author

I have implemented the tests in test file

@Aliqyan-21 Aliqyan-21 closed this May 29, 2023
@Aliqyan-21 Aliqyan-21 reopened this May 29, 2023
@Aliqyan-21
Copy link
Author

Sir,

I have implemented the clang-format successfully.

I also have implemented the tests in test/graaflib/algorithm/minimum_spanning_tree_test.cpp)

But I am getting an error while including minimum_spanning_tree.h in the test file, so I believe u may include it in your makefile or something to make it work, I apologise for not doing it myself.
I am pushing my code for now, Thank You.

@Aliqyan-21
Copy link
Author

Sir, I have made pull request implementing the test cases for scenerios like

  1. Empty graph
  2. Single vertex

@Aliqyan-21
Copy link
Author

  1. Linear graph
  2. Complete graph
  3. Graph with disconnected components

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.

Sir,

I have implemented the clang-format successfully.

I also have implemented the tests in test/graaflib/algorithm/minimum_spanning_tree_test.cpp)

But I am getting an error while including minimum_spanning_tree.h in the test file, so I believe u may include it in your makefile or something to make it work, I apologise for not doing it myself. I am pushing my code for now, Thank You.

Hi @Aliqyan-21, thanks for following up!

The CMake project is configured to automatically detect new files. Assuming you created a build directory in the root of your project, you can do the following to discover them:

# Discover the new source files and add them to targets
cmake ..

# Build the current project
cmake --build .

# Run the tests
ctest

Apart from that it looks like your build is failing as the path in your #include is off. Could you try changing it to the suggestion below to see if that works?

I will add some more clarification on this in the wiki. Also feel free to reach out on our discord if you encounter any more issues.

Comment on lines +3 to +4

#include "minimum_spanning_tree.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
#include "minimum_spanning_tree.h"
#include <graaflib/algorithm/minimum_spanning_tree.h>

Copy link
Author

Choose a reason for hiding this comment

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

#include <graaflib/algorithm/minimum_spanning_tree.h>

Getting this error again : - graaflib/algorithm/minimum_spanning_tree.h: No such file or directory

Comment on lines +12 to +13
// Helper struct for representing disjoint sets
struct DisjointSet {
Copy link
Owner

Choose a reason for hiding this comment

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

Could we put this in a namespace detail? I think it can also be moved to minimum_spanning_tree.tpp to keep the interface in the header file as clean as possible.

Copy link
Author

Choose a reason for hiding this comment

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

This I have done

Comment on lines +38 to +42

template <typename V, typename E, graph_spec S>
std::vector<std::pair<vertex_id_t, vertex_id_t>> minimum_spanning_tree(
const graph<V, E, S>& graph) {
using Edge = std::pair<edge_weight_t, std::pair<vertex_id_t, 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.

Same here, could you move the implementation to the .tpp file to keep you interface clean here? It would also be nice to have a short docstring on what this function does.

Maybe you could take a look at algorithm/graph_traversal.h for inspiration.

Copy link
Author

Choose a reason for hiding this comment

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

Done sir

@bobluppes
Copy link
Owner

FYI, I opened #27, which adds support for header-only installation of the library. When merged, this will introduce conflicts to any changed made under src/graaflib.

The conflicts should however be fairly simple to resolve, as the src directory was just renamed to include. Simply discard any changes made under src/graaflib and add them to the respective files under include/graaflib.

Feel free to reach out if you encounter any issues 🤖

@Aliqyan-21
Copy link
Author

@bobluppes I am still getting the error - graaflib/algorithm/minimum_spanning_tree.h: No such file or directory, after doing everything, but except this I am getting no error and code working fine

@bobluppes
Copy link
Owner

@bobluppes I am still getting the error - graaflib/algorithm/minimum_spanning_tree.h: No such file or directory, after doing everything, but except this I am getting no error and code working fine

Hi @Aliqyan-21, I suspect that your CMake configuration has not yet detected the new files. Could you try reconfiguring your CMake? From the command line this would be:

# Assuming you have a build folder at the root of the project
cd build

# Re-generate the build system
cmake ..

# Now trying to rebuild the project
cmake --build .

@bobluppes
Copy link
Owner

Hi @Aliqyan-21, closing this for now. If you want to continue on this in the future feel free to re-open.

More information on contributing and our development setup can be found on the wiki.

@bobluppes bobluppes closed this Jul 17, 2023
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.

None yet

2 participants