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

Weighted graph methods built for dynamic community detection #89

Merged
merged 10 commits into from Dec 7, 2021

Conversation

antepusic
Copy link
Collaborator

Weighted graph methods for MAGE’s dynamic community detection query module (#66), transcluded into separate PR for easier review per Matak’s suggestion.

@antepusic antepusic added lang: cpp Issue on C++ codebase type: utility Something that everyone should use hacktoberfest labels Oct 25, 2021
@antepusic antepusic self-assigned this Oct 25, 2021
cpp/mg_utility/mg_graph.hpp Outdated Show resolved Hide resolved
cpp/mg_utility/mg_generate.hpp Outdated Show resolved Hide resolved
cpp/mg_utility/mg_graph.hpp Show resolved Hide resolved
cpp/mg_utility/mg_graph.hpp Outdated Show resolved Hide resolved
@@ -279,6 +333,9 @@ class Graph : public GraphView<TSize> {

std::vector<std::vector<TSize>> adj_list_;
std::vector<std::vector<TNeighbour>> neighbours_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do neighbours_ contains both in and out neighbours or just out? If so make it explicit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Due to the way neighbours_ is initialized, with directed graphs it only contains the out-neighbors.
Keeping in mind that in- and out-neighbors are identical for undirected graphs, I’m reluctant to rename the vector, but I’ll clarify this with a comment. Does this sound good to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that sound ok for now. But I would definitely think about separating different versions of Graph, since now you have directed/undirected/weighted in the same class, and that causes undirected to have properties it doesn't use. What do you think about this @jmatak ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we decide to separate them, undirected graphs would still have to implement certain interfaces because of generality. For example, if an algorithm requires the out-neighbors, we should be able to call OutNeighbours without having to check whether the graph is directed. Of course, for undirected graphs, OutNeighbours would just default to calling Neighbours.

Does the above make sense? I believe we should definitely take it into account if it’s a valid suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

aha, so we are speaking in the context of algorithms running on both undirected and undirected graphs (sorry for the late response)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right @jbajic!

cpp/mg_utility/mg_utils.hpp Outdated Show resolved Hide resolved
@jmatak jmatak added the status: ship it PR approved label Nov 23, 2021
@jmatak jmatak merged commit cf40c47 into main Dec 7, 2021
@jmatak jmatak deleted the weighted_graph_support branch December 7, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang: cpp Issue on C++ codebase status: ship it PR approved type: utility Something that everyone should use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants