VF2 graph isomorphism revised PR for issue #87, requesting review #172
Conversation
…it was supposed to say internal
… the test and .tpp file
Codecov ReportAttention:
... and 56 files with indirect coverage changes 📢 Thoughts on this report? Let us know! |
|
Most of the added lines in PR are from cmake files i forgot to push because I rebuilt the library. |
|
@bobluppes @joweich could I get a workflow approval? |
sracha4355
force-pushed
the
satvik_graaf_pr_changes
branch
from
40df7af to
7c41e14
Compare
|
@joweich can I get a review |
There was a problem hiding this comment.
Hey @sracha4355, great to see you incorporated the feedback from last time!
I'll be a bit more specific with my feedback this time compared to the guardrails last time 🙂
- The logic itself is quite hard to follow because the level of abstraction changes often and some sections are quite repetitive. I think we should put some thought into introducing some object orientation to be more concise about the dependencies.
terminal_setorvf2_vertexwould be candidates to consider introducing as structs/classes - There are several return statements that lack test coverage. There should be tests if the returns are necessary, otherwise we should remove them
- You sometimes allocate memory with "naked" new statements. Most often, it's better (i.e. more robust against memory leaks) to use smart pointers. Won't go into detail here, but check e.g. this
I did not review every function, but I hope the bullets above and the inline-comments provide some useful advice already! 🙂
| template <typename V, typename E, graph_type T> | ||
| vf2_state* init_vf2_state(const graph<V, E, T>& lhs, | ||
| const graph<V, E, T>& rhs) { | ||
| struct vf2_state* state = new vf2_state; |
There was a problem hiding this comment.
| struct vf2_state* state = new vf2_state; | |
| std::unique_ptr<vf2_state> state = std::make_unique<vf2_state>(); |
Using smart pointers will ensure that the caller takes ownership of the returned objects and are thus less prone to memory leaks
| } | ||
|
|
||
| template <typename V, typename E, graph_type T> | ||
| vf2_state* init_vf2_state(const graph<V, E, T>& lhs, |
There was a problem hiding this comment.
This function's only purpose is initialization of the vf2_state struct. It can thus be treated as a constructor and moved into the struct's body as such.
| const graph<V, E, T>& rhs) { | ||
| if (!check_for_possibility_of_isomorphism(lhs, rhs)) return std::nullopt; | ||
|
|
||
| struct vf2_state* state = init_vf2_state(lhs, rhs); |
There was a problem hiding this comment.
| struct vf2_state* state = init_vf2_state(lhs, rhs); | |
| std::unique_ptr<vf2_state> state = init_vf2_state(lhs, rhs); |
| std::vector<std::size_t> lhs_node_degrees, rhs_node_degrees; | ||
| for (const auto& vertex : lhs.get_vertices()) { | ||
| std::size_t number_of_conn = lhs.get_neighbors(vertex.first).size(); | ||
| lhs_node_degrees.push_back(number_of_conn); | ||
| } | ||
| for (const auto& vertex : rhs.get_vertices()) { | ||
| std::size_t number_of_conn = rhs.get_neighbors(vertex.first).size(); | ||
| rhs_node_degrees.push_back(number_of_conn); | ||
| } | ||
| std::sort(lhs_node_degrees.begin(), lhs_node_degrees.end()); | ||
| std::sort(rhs_node_degrees.begin(), rhs_node_degrees.end()); | ||
| for (int i = 0; i < lhs_node_degrees.size(); i++) | ||
| if (lhs_node_degrees[i] != rhs_node_degrees[i]) return false; | ||
| return true; |
There was a problem hiding this comment.
| std::vector<std::size_t> lhs_node_degrees, rhs_node_degrees; | |
| for (const auto& vertex : lhs.get_vertices()) { | |
| std::size_t number_of_conn = lhs.get_neighbors(vertex.first).size(); | |
| lhs_node_degrees.push_back(number_of_conn); | |
| } | |
| for (const auto& vertex : rhs.get_vertices()) { | |
| std::size_t number_of_conn = rhs.get_neighbors(vertex.first).size(); | |
| rhs_node_degrees.push_back(number_of_conn); | |
| } | |
| std::sort(lhs_node_degrees.begin(), lhs_node_degrees.end()); | |
| std::sort(rhs_node_degrees.begin(), rhs_node_degrees.end()); | |
| for (int i = 0; i < lhs_node_degrees.size(); i++) | |
| if (lhs_node_degrees[i] != rhs_node_degrees[i]) return false; | |
| return true; | |
| auto compute_and_sort_degrees = [](const graph<V, E, T>& graph) { | |
| std::vector<std::size_t> node_degrees; | |
| for (const auto& vertex : graph.get_vertices()) { | |
| std::size_t number_of_conn = graph.get_neighbors(vertex.first).size(); | |
| node_degrees.push_back(number_of_conn); | |
| } | |
| std::sort(node_degrees.begin(), node_degrees.end()); | |
| return node_degrees; | |
| }; | |
| const auto lhs_node_degrees = compute_and_sort_degrees(lhs); | |
| const auto rhs_node_degrees = compute_and_sort_degrees(rhs); | |
| return lhs_node_degrees == rhs_node_degrees; |
I think using a lambda would be more concise here
| std::optional<vertex_mapping> _optional = mapping; | ||
| return _optional; |
There was a problem hiding this comment.
| std::optional<vertex_mapping> _optional = mapping; | |
| return _optional; | |
| return mapping; |
This should be possible with implicit template deduction.
| // a lookahead rule to see if the current partial vertex_mapping can lead to a | ||
| // complete isomorphic vertex_mapping | ||
| template <typename V, typename E, graph_type T> | ||
| bool r_new(const graph<V, E, T>& lhs, const graph<V, E, T>& rhs, |
There was a problem hiding this comment.
this function is very repetitive and its name should be more expressive
| size_t depth, size_t& _terminal_set_length) { | ||
| for (const auto& node : vertices) { | ||
| if (_terminal_set[node] == -1) { | ||
| _terminal_set[node] = int(depth); |
There was a problem hiding this comment.
why is depth defined as size_t when casted to int here?
|
@joweich thank you for your comments, I'll take a look at each one and refactor the code! |
|
Stale pull request message |
|
Hey guys, I got really busy with school, but now that I am on break I would like to refactor the code and get my code merged. Can we reopen the PR? |
Changes I've made:
I appreciate any constructive criticism you guys can offer. As for the comments, it is hard to tell the full story of the algorithm given it's complex nature, so in addition to my comments I linked the research paper if any devs are still confused.
Thank you guys!