Adding new utils header containing "get_transposed_graph" function #136

toadkarter · 2023-10-03T20:20:21Z

CHANGES

New overload for add_vertex() in the graph class has been added that allows user to specify an ID for a given vertex.
get_transposed_graph() function has been added, allowing the user to get a transposed version of a particular directed graph.
Both functions have a javadoc style comment explaining the interface.
Tests have been added to both functions.

Note: There does not seem to be a section in the documentation explaining the graph interface generally outside of the Quickstart guide, so I have not made any changes to the documentation. However, if something like this is required I would be happy to make additions.

If you would like me to make any changes please do let me know.

codecov · 2023-10-03T20:31:47Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
include/graaflib/graph.h	`100.00% <ø> (ø)`
include/graaflib/graph.tpp	`100.00% <100.00%> (ø)`
test/graaflib/algorithm/utils_test.cpp	`100.00% <100.00%> (ø)`
test/graaflib/graph_test.cpp	`100.00% <100.00%> (ø)`
include/graaflib/algorithm/utils.tpp	`92.85% <92.85%> (ø)`

... and 16 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

bobluppes

Nice, looks quite good already, hereby some early feedback

bobluppes · 2023-10-05T20:02:13Z

include/graaflib/graph.h

+/**
+ * Get transposed version of a given directed graph
+ *
+ * @param  graph The directed graph that is to be transposed
+ * @param  out_transposed_graph An out parameter that will contain the
+ * transposed graph
+ */
+template <typename VERTEX_T, typename EDGE_T>
+void get_transposed_graph(
+    const directed_graph<VERTEX_T, EDGE_T>& graph,
+    directed_graph<VERTEX_T, EDGE_T>& out_transposed_graph);


What do you think about moving this to a separate source file include/graaflib/algorithm/utils.h?

What do you think about moving this to a separate source file include/graaflib/algorithm/utils.h?

Brill, yeah I think that makes sense. That way we are not polluting the graph headers with auxiliary files that the users might not not need.

bobluppes · 2023-10-05T20:03:54Z

test/graaflib/graph_test.cpp

+TEST(GraphTest, Transpose) {
+  // GIVEN
+  using graph_t = directed_graph<int, int>;
+  graph_t graph{};
+  const auto vertex_id_1 = graph.add_vertex(1);


Same for this test, we can then also move it to its own file

No prob, will do!

bobluppes · 2023-10-05T20:06:42Z

include/graaflib/graph.h

+template <typename VERTEX_T, typename EDGE_T>
+void get_transposed_graph(
+    const directed_graph<VERTEX_T, EDGE_T>& graph,
+    directed_graph<VERTEX_T, EDGE_T>& out_transposed_graph);


Instead of having out_transposed_graph an output parameter, I would rather use a return value:

template <typename V, typename E> directed_graph<V, E> get_transposed_graph(const directed_graph<V, E>& graph) {...}

No problem! Will make this tweak

bobluppes · 2023-10-05T20:12:24Z

include/graaflib/graph.tpp

+
+    if (!out_transposed_graph.has_vertex(vertex_id_lhs)) {


What is the point of these checks? If the out graph already contains these vertices, shouldn't we throw an exception? Or better, could we construct a new empty graph in the function body and use that as the return value?

Then we can ensure ourselves that a vertex with that ID is not there yet

What is the point of these checks? If the out graph already contains these vertices, shouldn't we throw an exception? Or better, could we construct a new empty graph in the function body and use that as the return value?

Then we can ensure ourselves that a vertex with that ID is not there yet

I think what I was trying to get at here was a situation like this (in pseudocode):

NewGraph={} Original Graph Edges = Edge(Vertex 1, Vertex 2) and Edge(Vertex 2, Vertex3) for Edge in Edges: NewGraph.Add(Edge.LeftVertex) NewGraph.Add(Edge.RightVertex)

In the above example, when we examine the second edge, we will encounter Vertex 2 again, because we have already added it when looping through the first edge. This is not an error, because we expect to find certain vertices again while looping if the edges are connected, so I'm not sure if using an exception makes sense here?

But I definitely agree on constructing an empty graph as the return value!

Ah yes, that makes sense then. I was thinking about how the code was also checking whether the passed in graph did not contain the IDs before we do any computation, but this will not be a problem once we construct the graph in the body of the function 👍🏻

Ah yes, that makes sense then. I was thinking about how the code was also checking whether the passed in graph did not contain the IDs before we do any computation, but this will not be a problem once we construct the graph in the body of the function 👍🏻

Great stuff! Those tweaks should now all be pushed. Would you like me to squash all the commits or are you happy for these to stay separate for the time being?

bobluppes · 2023-10-05T20:13:19Z

include/graaflib/graph.tpp

+    if (!out_transposed_graph.has_vertex(vertex_id_lhs)) {
+      auto vertex_id =
+          out_transposed_graph.add_vertex(vertex_value_lhs, vertex_id_lhs);
+    }
+
+    if (!out_transposed_graph.has_vertex(vertex_id_rhs)) {
+      auto vertex_id =
+          out_transposed_graph.add_vertex(vertex_value_rhs, vertex_id_rhs);
+    }


Since the vertex value could be expensive to copy, we could use moves here 😉

Suggested change

if (!out_transposed_graph.has_vertex(vertex_id_lhs)) {

auto vertex_id =

out_transposed_graph.add_vertex(vertex_value_lhs, vertex_id_lhs);

}

if (!out_transposed_graph.has_vertex(vertex_id_rhs)) {

auto vertex_id =

out_transposed_graph.add_vertex(vertex_value_rhs, vertex_id_rhs);

}

if (!out_transposed_graph.has_vertex(vertex_id_lhs)) {

auto vertex_id =

out_transposed_graph.add_vertex(std::move(vertex_value_lhs), vertex_id_lhs);

}

if (!out_transposed_graph.has_vertex(vertex_id_rhs)) {

auto vertex_id =

out_transposed_graph.add_vertex(std::move(vertex_value_rhs), vertex_id_rhs);

}

Awesome, love this! Will add.

bobluppes

Thanks for working on this, LGTM!

Just some minor comments, let me know what you think about them and I would be happy to merge :)

As a final thought for future work, I was thinking about what behavior we would expect if I transposed an unconnected graph. Let's say I have graph A with vertices 1, 2, and 3. A only has the directed edge 1 -> 2. Currently if we compute the transpose of A, 3 would not show up in the result set.

What I think we could do in a future implementation is to use the copy constructor for the entire graph class. Then we can create a copy, after which we iterate over all edges in the copy. For every edge we can then call remove_edge and add_edge with the source and target vertices reversed. Wdyt?

bobluppes · 2023-10-07T21:01:08Z

include/graaflib/graph.h

+   * @throws id_taken exception - If the relevant ID is already in use
+   */
+  [[nodiscard]] vertex_id_t add_vertex(auto&& vertex, vertex_id_t id);


[very minor]: Since the user already knows the ID of the vertex, it is not necessary to mark the return value as nodiscard since the user can safely ignore it.

Suggested change

* @throws id_taken exception - If the relevant ID is already in use

*/

[[nodiscard]] vertex_id_t add_vertex(auto&& vertex, vertex_id_t id);

* @throws id_taken exception - If the relevant ID is already in use

*/

vertex_id_t add_vertex(auto&& vertex, vertex_id_t id);

bobluppes · 2023-10-07T21:02:14Z

include/graaflib/graph.h

+/**
+ * Get transposed version of a given directed graph
+ *
+ * @param  graph The directed graph that is to be transposed
+ * @param  out_transposed_graph An out parameter that will contain the
+ * transposed graph
+ */
+template <typename VERTEX_T, typename EDGE_T>
+void get_transposed_graph(


I think this is left-over code now, as the implementation now lives in algorithm/utils.h

bobluppes · 2023-10-07T21:06:16Z

include/graaflib/graph.tpp

+  while (has_vertex(vertex_id_supplier_)) {
+    vertex_id_supplier_++;
+  }
+  const auto vertex_id{vertex_id_supplier_};


[very minor]: Instead of post-increment (vertex_id_supplier_++) we could use pre-increment here (++vertex_id_supplier). This is slightly faster as post-increment returns the old value while pre-increment returns the new value. Therefore pre-increment does not need to keep a temporary for the old value.

Since we are not using the return value anyway we can safely do this (arguably very small) optimization :)

bobluppes · 2023-10-07T21:11:10Z

test/graaflib/graph_test.cpp

+  // WHEN - THEN
+  constexpr int colliding_id = 2;
+  const auto vertex_id_4{graph.add_vertex(40)};
+  ASSERT_EQ(graph.vertex_count(), 4);
+  ASSERT_TRUE(graph.has_vertex(colliding_id + 1));
+  ASSERT_EQ(graph.get_vertex(colliding_id), 30);
+  ASSERT_EQ(graph.get_vertex(colliding_id + 1), 40);


In this test section, aren't we also dependent on the implementation specific vertex ID logic since we are checking whether the graph has a vertex with ID colliding_id + 1. I remember the original reason for the new add_vertex methods was to not be dependent on this implementation detail.

Would it also make sense to get rid of this WHEN - THEN section then?

In this test section, aren't we also dependent on the implementation specific vertex ID logic since we are checking whether the graph has a vertex with ID colliding_id + 1. I remember the original reason for the new add_vertex methods was to not be dependent on this implementation detail.

Would it also make sense to get rid of this WHEN - THEN section then?

If I understand correctly, do you mean that this portion of the test might need to be changed down the line because we are relying specifically on how the id is calculated? In that case, I think it's fair to say that this section is too reliant on how add_vertex is implemented! I will remove in the next commit

Exactly, if we change the underlying ID generation logic this test would break.

I would say that the newly added method is already sufficiently covered so we can safely remove this part

bobluppes · 2023-10-07T21:15:25Z

include/graaflib/algorithm/utils.h

+
+template <typename VERTEX_T, typename EDGE_T>
+directed_graph<VERTEX_T, EDGE_T> get_transposed_graph(


[very minor]: In the codebase we have the habit of splitting definition and implementation. Maybe we can add a utils.tpp file for consistency. I would however also be fine to do this in a separate PR as the current one unblocks another PR.

toadkarter · 2023-10-07T21:47:11Z

Thanks for working on this, LGTM!

Just some minor comments, let me know what you think about them and I would be happy to merge :)

As a final thought for future work, I was thinking about what behavior we would expect if I transposed an unconnected graph. Let's say I have graph A with vertices 1, 2, and 3. A only has the directed edge 1 -> 2. Currently if we compute the transpose of A, 3 would not show up in the result set.

What I think we could do in a future implementation is to use the copy constructor for the entire graph class. Then we can create a copy, after which we iterate over all edges in the copy. For every edge we can then call remove_edge and add_edge with the source and target vertices reversed. Wdyt?

Thanks very much for taking the time to review the PR, much appreciated! I will incorporate your commits and do an additional push, squashing the commits at the same time just to make sure that we have one clean commit going into the codebase. I only have one question on these comments which I will put into the relevant thread.

toadkarter · 2023-10-07T21:54:36Z

As for unconnected graphs - that's actually a great point, I haven't thought of that. I would be happy to have a look at that once this and Kosaraju's have gone in!

bobluppes · 2023-10-07T22:14:23Z

Thanks for working on this, LGTM!

Just some minor comments, let me know what you think about them and I would be happy to merge :)

As a final thought for future work, I was thinking about what behavior we would expect if I transposed an unconnected graph. Let's say I have graph A with vertices 1, 2, and 3. A only has the directed edge 1 -> 2. Currently if we compute the transpose of A, 3 would not show up in the result set.

What I think we could do in a future implementation is to use the copy constructor for the entire graph class. Then we can create a copy, after which we iterate over all edges in the copy. For every edge we can then call remove_edge and add_edge with the source and target vertices reversed. Wdyt?

Thanks very much for taking the time to review the PR, much appreciated! I will incorporate your commits and do an additional push, squashing the commits at the same time just to make sure that we have one clean commit going into the codebase. I only have one question on these comments which I will put into the relevant thread.

Thanks, sounds great!
As a final remark, we have "squash and merge" enabled, so all main merges are squashed by default. Therefore no need to manually do this on a PR. But also no harm done if you did this already of course :)

bobluppes

Thanks for working on this, LGTM!

toadkarter marked this pull request as ready for review October 3, 2023 21:23

bobluppes reviewed Oct 5, 2023

View reviewed changes

bobluppes approved these changes Oct 7, 2023

View reviewed changes

bobluppes added enhancement New feature hacktoberfest-accepted labels Oct 7, 2023

toadkarter changed the title ~~Adding new transpose function to the public graph interface.~~ Adding new utils header containing "get_transposed_graph" function Oct 7, 2023

Adds new utils header containing get_transposed_graph function.

cb9f53c

toadkarter force-pushed the transpose-function branch from 6558687 to cb9f53c Compare October 7, 2023 22:22

Updating javadoc on get_transposed_graph

d378c81

bobluppes approved these changes Oct 8, 2023

View reviewed changes

bobluppes merged commit ae3e232 into bobluppes:main Oct 8, 2023
8 checks passed

toadkarter deleted the transpose-function branch October 8, 2023 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new utils header containing "get_transposed_graph" function #136

Adding new utils header containing "get_transposed_graph" function #136

toadkarter commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

bobluppes left a comment

bobluppes Oct 5, 2023

toadkarter Oct 5, 2023

bobluppes Oct 5, 2023

toadkarter Oct 5, 2023

bobluppes Oct 5, 2023

toadkarter Oct 5, 2023

bobluppes Oct 5, 2023

toadkarter Oct 5, 2023

bobluppes Oct 7, 2023

toadkarter Oct 7, 2023

bobluppes Oct 5, 2023

toadkarter Oct 5, 2023

bobluppes left a comment

bobluppes Oct 7, 2023

bobluppes Oct 7, 2023

bobluppes Oct 7, 2023

bobluppes Oct 7, 2023

toadkarter Oct 7, 2023

bobluppes Oct 7, 2023

bobluppes Oct 7, 2023

toadkarter commented Oct 7, 2023

toadkarter commented Oct 7, 2023

bobluppes commented Oct 7, 2023

bobluppes left a comment


		template <typename VERTEX_T, typename EDGE_T>
		directed_graph<VERTEX_T, EDGE_T> get_transposed_graph(

Adding new utils header containing "get_transposed_graph" function #136

Adding new utils header containing "get_transposed_graph" function #136

Conversation

toadkarter commented Oct 3, 2023 • edited Loading

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

bobluppes 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobluppes 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

toadkarter commented Oct 7, 2023

toadkarter commented Oct 7, 2023

bobluppes commented Oct 7, 2023

bobluppes left a comment

Choose a reason for hiding this comment

toadkarter commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading