-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: Vertex degree [free function version] #29
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
+ Coverage 99.84% 99.86% +0.02%
==========================================
Files 16 18 +2
Lines 655 754 +99
==========================================
+ Hits 654 753 +99
Misses 1 1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks excellent!
Some smaller comments in case you want to take a look, but can also do that in a follow-up.
Just give me the 👍🏻 and I am happy to merge
int indegree{0}; | ||
for (const auto& current_vertex : graph.get_vertices()) { | ||
if (graph.get_neighbors(current_vertex.first).contains(vertex_id)) { | ||
indegree++; | ||
} | ||
} | ||
return indegree; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on whether we expect graphs to be spart/dense it might also be worth to loop over the edges. I would propose to leave this as-is for now until we have some actual usage though.
If you are up for it, a slightly more modern C++ experience would be to use std::algorithm
. However I can also do this in a follow-up.
Iterating over all vertices
// Requires including <algorithm>
return std:ranges::count_if(graph.get_vertices(), [&graph, vertex_id](const vertex_id_to_vertex_t::value_type& kv_pair) {
const auto& [current_vertex_id, _]{kv_pair};
return graph.get_neighbors(current_vertex_id).contains(vertex_id);
});
Iterating over all edges
// Requires including <algorithm>
return std::ranges::count_if(graph.get_edges(), [vertex_id](const vertex_ids_to_edge_t::value_type& kv_pair) {
const auto& [edge_id, _]{kv_pair};
return edge_id.first == vertex_id || edge_id.second == vertex_id;
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on whether we expect graphs to be spart/dense it might also be worth to loop over the edges. I would propose to leave this as-is for now until we have some actual usage though.
I think it's hard to come up with a generic approach here. Depending on the graph architecture, the one or the other might be faster.
If you are up for it, a slightly more modern C++ experience would be to use std::algorithm.
Damn, I feel this wasn't there in C++14. Covered in 50d78ac.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks!
Yes, luckily we have C++20 now 😉
See thread in #24 for details