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

Issue: #419, Issue: #238, and Issue: #240 #422

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions include/CXXGraph/Graph/Algorithm/Dinic_impl.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#include <iostream>
#include <queue>
#include <limits>
#include <unordered_map>
#include <vector>
#include "Node_impl.h"
#include "CXXGraph/Graph/Graph_decl.h"


namespace CXXGraph {

template <typename T>
class Graph {
private:
std::unordered_map<CXXGraph::id_t, Node<T>> nodes;
Copy link
Owner

Choose a reason for hiding this comment

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

These should not be a class variable, reduce the scope of the variable to the minimum required

std::unordered_map<CXXGraph::id_t, std::unordered_map<CXXGraph::id_t, int>> capacities;
std::unordered_map<CXXGraph::id_t, std::unordered_map<CXXGraph::id_t, int>> flow;
Comment on lines +16 to +17
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this adds more storage to the graph, which makes it a larger object/heavier. Thoughts on this @sbaldu, @ZigRazor? Perhaps the relevant storage of the algorithm should be extracted elsewhere (and possibly the algorithm implementation itself) so that this storage is temporary.

There are other options, such as lazy storage creation on the heap the first time a dfs, bfs, or Dinic's is called. Although the storage for std::unordered_map isn't wild - I do like keeping Graph as lightweight as can possibly be done.

Copy link
Owner

Choose a reason for hiding this comment

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

I Agree!

Comment on lines +12 to +17
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, why is he re-declaring the Graph class? Shouldn't he simply add the declaration of the method in Graph_decl.hpp and add a header for the implementation of the single method?

Copy link
Owner

Choose a reason for hiding this comment

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

I Agree!


bool bfs(const CXXGraph::id_t& source, const CXXGraph::id_t& sink, std::unordered_map<CXXGraph::id_t, int>& distance) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why adding a new bfs implementation? can you use the provided one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for implementing bfs and dfs (and Dinic's!).

These actually seem generally useful. Should we consider making them public? (assuming the storage requirement would need to change given the above comment).

for (auto& pair : nodes) {
distance[pair.first] = -1;
}
distance[source] = 0;

std::queue<CXXGraph::id_t> q;
q.push(source);

while (!q.empty()) {
CXXGraph::id_t current = q.front();
q.pop();
for (auto& [neighbor_id, capacity] : capacities[current]) {
if (distance[neighbor_id] == -1 && flow[current][neighbor_id] < capacity) {
distance[neighbor_id] = distance[current] + 1;
q.push(neighbor_id);
}
}
}

return distance[sink] != -1;
}

int dfs(const CXXGraph::id_t& current, const CXXGraph::id_t& sink, int flow_value, std::unordered_map<CXXGraph::id_t, int>& distance) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why adding a new dfs implementation? can you use the provided one?

if (current == sink) {
return flow_value;
}

for (auto& [neighbor_id, capacity] : capacities[current]) {
if (distance[neighbor_id] == distance[current] + 1 && flow[current][neighbor_id] < capacity) {
int new_flow = dfs(neighbor_id, sink, std::min(flow_value, capacity - flow[current][neighbor_id]), distance);
if (new_flow > 0) {
flow[current][neighbor_id] += new_flow;
flow[neighbor_id][current] -= new_flow;
return new_flow;
}
}
}

return 0;
}

public:
// Other graph methods...

// Function to add an edge with capacity
void addEdge(const std::string& from_id, const std::string& to_id, int capacity) {
capacities[from_id][to_id] = capacity;
flow[from_id][to_id] = 0;
// Assuming nodes are already added to the graph
}

// Dinic's algorithm to find maximum flow
int dinicsAlgorithm(const CXXGraph::id_t& source, const CXXGraph::id_t& sink) {
int max_flow = 0;
std::unordered_map<CXXGraph::id_t, int> distance;

while (bfs(source, sink, distance)) {
int path_flow = 0;
while ((path_flow = dfs(source, sink, std::numeric_limits<int>::max(), distance)) != 0) {
max_flow += path_flow;
}
}

return max_flow;
}
};

} // namespace CXXGraph
66 changes: 66 additions & 0 deletions include/CXXGraph/Graph/Algorithm/Krager_impl.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#include <iostream>
#include <unordered_map>
#include <unordered_set>
#include <vector>
#include <random>
#include "Node_impl.h"
#include "CXXGraph/Graph/Graph_decl.h"


namespace CXXGraph {

template <typename T>
class Graph {
private:
std::unordered_map<CXXGraph::id_t, Node<T>> nodes;
Copy link
Owner

Choose a reason for hiding this comment

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

in Graph_decl.h we already have the node set and the adjiacency matrix, you should use them

std::unordered_map<CXXGraph::id_t, std::unordered_set<CXXGraph::id_t>> adjacency_list;

public:
// Other graph methods...

// Function to add an edge between two nodes
void addEdge(const std::string& from_id, const std::string& to_id) {
Copy link
Owner

Choose a reason for hiding this comment

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

this function is a repetition of the already implemented function in Graph_impl.hpp, you should use it

adjacency_list[from_id].insert(to_id);
adjacency_list[to_id].insert(from_id);
// Assuming nodes are already added to the graph
}

// Krager's algorithm for graph partitioning
std::pair<std::unordered_set<CXXGraph::id_t>, std::unordered_set<CXXGraph::id_t>> kragersAlgorithm() {
// Initialize clusters
std::unordered_set<CXXGraph::id_t> cluster1;
std::unordered_set<CXXGraph::id_t> cluster2;
for (const auto& node_pair : nodes) {
cluster1.insert(node_pair.first);
}

// Perform Krager's algorithm
std::random_device rd;
std::mt19937 gen(rd());
while (cluster1.size() > 2) {
// Select a random edge
std::uniform_int_distribution<size_t> dist(0, adjacency_list.size() - 1);
size_t rand_index = dist(gen);
auto it = std::next(std::begin(adjacency_list), rand_index);
CXXGraph::id_t u = it->first;
auto& neighbors = it->second;
std::uniform_int_distribution<size_t> dist_neighbor(0, neighbors.size() - 1);
size_t rand_neighbor_index = dist_neighbor(gen);
auto neighbor_it = std::next(std::begin(neighbors), rand_neighbor_index);
CXXGraph::id_t v = *neighbor_it;

// Merge clusters if the vertices belong to different clusters
if (cluster1.count(u) && cluster1.count(v)) {
cluster1.erase(v);
cluster2.insert(v);
} else if (cluster2.count(u) && cluster1.count(v)) {
cluster1.erase(v);
cluster2.insert(v);
}
}

return {cluster1, cluster2};
}
};

} // namespace CXXGraph
1 change: 1 addition & 0 deletions include/CXXGraph/Node/Node_decl.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class Node {
const std::string &getUserId() const;
const T &getData() const;
void setData(T &&new_data);

// operator
bool operator==(const Node<T> &b) const;
bool operator<(const Node<T> &b) const;
Expand Down
12 changes: 12 additions & 0 deletions include/CXXGraph/Node/Node_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,18 @@ void Node<T>::setData(T &&new_data) {
this->data = std::move(new_data);
}

bool deleteNodeById(const std::string &id) {
auto it = std::find_if(nodes.begin(), nodes.end(), [&](const auto &pair) {
return pair.second.getUserId() == id;
});
Comment on lines +73 to +76
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to include algorithm in order to use std::find_if

Copy link
Owner

Choose a reason for hiding this comment

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

YES


if (it != nodes.end()) {
nodes.erase(it);
return true; // Node deleted successfully
}
return false; // Node not found
}
Comment on lines +73 to +83
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯 🙏


// The data type T must have an overload of the equality operator
template <typename T>
bool Node<T>::operator==(const Node<T> &b) const {
Expand Down
Loading