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

Saj18 · 2024-04-30T00:06:30Z

#419
-Implemented the deleteNodeById function in the Graph class to delete a node by its ID.
-Utilized a lambda function with std::find_if to search for the node with the provided ID within the unordered map of nodes.
-If the node is found, it is erased from the map using nodes.erase(it).
-Returns true if the node is successfully deleted and false if the node is not found.

#238

Implemented Dinic's algorithm for finding the maximum flow in a flow network.
Added necessary methods and data structures to the Graph class.
Implemented BFS and DFS helper functions for the algorithm.

#240

Implemented Krager's algorithm for partitioning a graph into two clusters.
Added the kragersAlgorithm method to the Graph class.
Added necessary data structures and utilities to support the algorithm.
Utilized randomization for edge selection within the algorithm.

Issue: ZigRazor#419

Issue: ZigRazor#238

Issue: ZigRazor#240

ZigRazor

Please resolve the comment and we can merge it. Good Work, just some details to adjust!

ZigRazor · 2024-04-30T07:14:28Z

include/CXXGraph/Graph/Algorithm/Dinic_impl.hpp

+    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;
+
+    bool bfs(const CXXGraph::id_t& source, const CXXGraph::id_t& sink, std::unordered_map<CXXGraph::id_t, int>& distance) {


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

ZigRazor · 2024-04-30T07:14:39Z

include/CXXGraph/Graph/Algorithm/Dinic_impl.hpp

+        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) {


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

ZigRazor · 2024-04-30T07:17:47Z

include/CXXGraph/Graph/Algorithm/Dinic_impl.hpp

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


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

ZigRazor · 2024-04-30T07:19:02Z

include/CXXGraph/Graph/Algorithm/Krager_impl.hpp

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


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

ZigRazor · 2024-04-30T07:20:20Z

include/CXXGraph/Graph/Algorithm/Krager_impl.hpp

+    // Other graph methods...
+
+    // Function to add an edge between two nodes
+    void addEdge(const std::string& from_id, const std::string& to_id) {


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

nolankramer · 2024-04-30T07:16:10Z

include/CXXGraph/Graph/Algorithm/Dinic_impl.hpp

+    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;
+
+    bool bfs(const CXXGraph::id_t& source, const CXXGraph::id_t& sink, std::unordered_map<CXXGraph::id_t, int>& distance) {


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).

nolankramer · 2024-04-30T07:17:45Z

include/CXXGraph/Graph/Algorithm/Dinic_impl.hpp

+    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;


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.

nolankramer · 2024-04-30T07:21:33Z

include/CXXGraph/Node/Node_impl.hpp

+ bool deleteNodeById(const std::string &id) {
+    auto it = std::find_if(nodes.begin(), nodes.end(), [&](const auto &pair) {
+      return pair.second.getUserId() == id;
+    });
+
+    if (it != nodes.end()) {
+      nodes.erase(it);
+      return true; // Node deleted successfully
+    }
+    return false; // Node not found
+  }


ZigRazor · 2024-04-30T07:22:59Z

Another things, can you add tests for your algorithm implementation and for the node search by ID?
Thank you in advance!

nolankramer · 2024-04-30T07:29:24Z

My comments largely reflect @ZigRazor's comments, so mine can be ignored.

sbaldu · 2024-04-30T07:42:21Z

include/CXXGraph/Node/Node_impl.hpp

+ bool deleteNodeById(const std::string &id) {
+    auto it = std::find_if(nodes.begin(), nodes.end(), [&](const auto &pair) {
+      return pair.second.getUserId() == id;
+    });


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

sbaldu · 2024-04-30T12:23:42Z

include/CXXGraph/Graph/Algorithm/Dinic_impl.hpp

+template <typename T>
+class Graph {
+private:
+    std::unordered_map<CXXGraph::id_t, Node<T>> nodes;
+    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;


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?

Sab321 and others added 3 commits April 29, 2024 18:35

Add API to remove nodes from CXXGraph by id

53b9961

Issue: ZigRazor#419

Add Dinic's Algorithm

71b6b4d

Issue: ZigRazor#238

Add Karger's Algorithm for partitioning a graph

40ff2b3

Issue: ZigRazor#240

Saj18 changed the title ~~Issue: #419 and Issue: #238~~ Issue: #419, Issue: #238, and Issue: #240 Apr 30, 2024

Added include

8b1ae1a

ZigRazor requested review from ZigRazor, sbaldu and nolankramer and removed request for ZigRazor April 30, 2024 07:12

ZigRazor requested changes Apr 30, 2024

View reviewed changes

This was linked to issues Apr 30, 2024

Add API to remove nodes from CXXGraph by id #419

Open

Add Dinic's Algorithm #238

Open

add Karger's Algorithm for partitioning a graph. #240

Open

nolankramer reviewed Apr 30, 2024

View reviewed changes

ZigRazor self-assigned this Apr 30, 2024

ZigRazor added enhancement New feature or request development Development of new Functionalities core something about core labels Apr 30, 2024

ZigRazor added this to the Algorithm Implementation milestone Apr 30, 2024

sbaldu requested changes Apr 30, 2024

View reviewed changes

sbaldu reviewed Apr 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Saj18 commented Apr 30, 2024 •

edited

ZigRazor left a comment

ZigRazor Apr 30, 2024

ZigRazor Apr 30, 2024

ZigRazor Apr 30, 2024

ZigRazor Apr 30, 2024

ZigRazor Apr 30, 2024

nolankramer Apr 30, 2024

nolankramer Apr 30, 2024

ZigRazor Apr 30, 2024

nolankramer Apr 30, 2024

ZigRazor commented Apr 30, 2024

nolankramer commented Apr 30, 2024 •

edited

sbaldu Apr 30, 2024

ZigRazor May 2, 2024

sbaldu Apr 30, 2024

ZigRazor May 2, 2024

		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;

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

Are you sure you want to change the base?

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

Conversation

Saj18 commented Apr 30, 2024 • edited

ZigRazor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZigRazor commented Apr 30, 2024

nolankramer commented Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saj18 commented Apr 30, 2024 •

edited

nolankramer commented Apr 30, 2024 •

edited