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

Dynamic betweenness centrality #127

Merged
merged 20 commits into from Apr 15, 2022
Merged

Dynamic betweenness centrality #127

merged 20 commits into from Apr 15, 2022

Conversation

antepusic
Copy link
Collaborator

@antepusic antepusic commented Mar 15, 2022

Description

Dynamic betweenness centrality implementation.

Pull request type

  • Algorithm/Module

######################################

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

  • Core algorithm/module implementation
  • Query module implementation
  • Unit tests
  • End-to-end tests
  • Code documentation
  • README short description
  • Documentation on memgraph/docs

######################################

@antepusic antepusic self-assigned this Mar 15, 2022
@antepusic antepusic requested a review from jmatak March 15, 2022 08:03
@antepusic antepusic marked this pull request as draft March 15, 2022 08:04
@antepusic antepusic added lang: cpp Issue on C++ codebase status: draft PR is in draft phase type: module type: algorithm Initial algorithm implementation labels Mar 15, 2022
Copy link
Contributor

@jmatak jmatak left a comment

Choose a reason for hiding this comment

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

This is the first par of the review

Copy link
Contributor

@jmatak jmatak left a comment

Choose a reason for hiding this comment

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

Just some idea for the E2E tests. Try out different examples with NetworkX and create test with Python

cpp/betweenness_centrality_module/CMakeLists.txt Outdated Show resolved Hide resolved
// Dynamically update betweenness centrality scores by each created edge
for (const auto created_edge : created_edges) {
edges.push_back({graph->GetInnerNodeId(created_edge.first), graph->GetInnerNodeId(created_edge.second)});
graph = mg_generate::BuildGraph(graph->Nodes().size(), edges, mg_graph::GraphType::kUndirectedGraph);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks extremely computationally heavy

Copy link
Collaborator Author

@antepusic antepusic Mar 17, 2022

Choose a reason for hiding this comment

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

The alternative is using the CreateEdge and EraseEdge methods. Whereas that will not mess up the inner IDs (with edge update, both the prior and the current graph have the same nodes), I'd appreciate a greenlight from you before going ahead with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we've discussed this separately, please make a TODO for that somewhere :D

Comment on lines 104 to 118
if (created_node_ids.size() == 0 && deleted_node_ids.size() == 0) { // Use the online algorithm
// Construct the graph as before the update
std::vector<std::pair<std::uint64_t, std::uint64_t>> edges;
edges.reserve(graph->Edges().size());
for (const auto edge : graph->Edges()) {
std::pair<uint64_t, uint64_t> edge_memgraph_ids = {graph->GetMemgraphNodeId(edge.from),
graph->GetMemgraphNodeId(edge.to)};
// Newly created edges aren’t part of the prior graph
if (std::find(created_edges.begin(), created_edges.end(), edge_memgraph_ids) == created_edges.end())
edges.push_back({edge.from, edge.to});
}
for (const auto edge : deleted_edges) {
edges.push_back({graph->GetInnerNodeId(edge.first), graph->GetInnerNodeId(edge.second)});
}
auto prior_graph = mg_generate::BuildGraph(graph->Nodes().size(), edges, mg_graph::GraphType::kUndirectedGraph);
Copy link
Contributor

Choose a reason for hiding this comment

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

Candidate for mg_utility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've understood that our utilities all get information from the current graph about its composition, and this code snippet is different in that it takes that data (the Edges call) and then transforms it. With this in mind, what would you suggest? 🤔

@antepusic antepusic requested review from jbajic and jmatak March 18, 2022 15:40
@antepusic antepusic added status: ready PR is ready for review and removed status: draft PR is in draft phase labels Mar 27, 2022
@antepusic antepusic marked this pull request as ready for review April 6, 2022 07:59
cpp/mg_utility/data_structures/graph_view.hpp Outdated Show resolved Hide resolved
cpp/mg_utility/mg_test_utils.hpp Show resolved Hide resolved
Comment on lines 58 to 70
///@brief Performs a breadth-first search from a given node with the Brandes’ algorithm.
///
///@param graph Graph traversed the BFS
///@param start_id Start node ID
///@param compensate_for_deleted_node If true, guarantees correct results for the DETACH_DELETE_NODE operation
///
///@return Nº of shortest paths to each node from the start node
/// Each node’s immediate predecessors on the shortest paths from the start node
/// IDs of nodes visited by the BFS, in reverse order
std::tuple<std::unordered_map<std::uint64_t, std::uint64_t>,
std::unordered_map<std::uint64_t, std::set<std::uint64_t>>, std::vector<std::uint64_t>>
BrandesBFS(const mg_graph::GraphView<> &graph, const std::uint64_t start_id,
const bool compensate_for_deleted_node = false) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where exactly do we use the information from the Brandes' algorithm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is used in the cases handled by NodeEdgeUpdate, i.e. those where a single iteration of Brandes is enough to update BC values.

Comment on lines +89 to +91
std::unordered_map<std::uint64_t, std::uint64_t> SSSPLengths(
const mg_graph::GraphView<> &graph, const std::uint64_t start_id,
const std::unordered_set<std::uint64_t> &bcc_nodes) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be encapsulated in one of the BFS methods to not repeat the process, or it would be too much work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any of the BFS methods could be used for this, but at cost of making unnecessary data structures (e.g. predecessors, Ns of shortest paths) when the algorithm only needs the distances.

Copy link
Contributor

@jmatak jmatak left a comment

Choose a reason for hiding this comment

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

🎉

@jmatak jmatak merged commit 92f86eb into main Apr 15, 2022
@jmatak jmatak deleted the T080-MAGE-dynamic-BC branch April 15, 2022 13:29
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: ready PR is ready for review type: algorithm Initial algorithm implementation type: module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants