Requesting a code review of the VF2 Algorithm for ISSUE: 87 #168

sracha4355 · 2023-10-25T00:50:44Z

The algorithm is tested and it works well. I still need to add better comments for the code and the tests, but I would like a initial code review for graph_isomorphism.h and graph_isomorpishm.tpp (please ignore the comments in graph_isomorphism.tpp for now). The VF2 algorithm in addition to comparing the structure of the graph for isomorphism also allows for semantic comparison of graphs, meaning the attributes of the nodes. However, this code only takes into account the structure of the graphs. @bobluppes I was wondering if we can push this working code and add on this functionality later on? Thank you, please be brutal with my code as is my first time really working with C++ outside of school.
P.S how do I check code coverage?

…it was supposed to say internal

… the test and .tpp file

github-actions

Hi there! Thank you for creating your first pull-request on the Graaf library :)

codecov · 2023-10-25T08:51:40Z

Codecov Report

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

Files	Coverage Δ
include/graaflib/graph.h	`100.00% <ø> (ø)`
test/graaflib/algorithm/graph_isomorphism.cpp	`100.00% <100.00%> (ø)`
include/graaflib/algorithm/graph_isomorphism.tpp	`95.84% <95.84%> (ø)`

... and 56 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

joweich

Hey @sracha4355, huge congrats on your very first outside-of-school C++ PR 🚀 You definitely chose a challenging one for this and did very well!
Some early guardrails for increasing compliance with best practices and making your code more concise:

In the header file, you should only have the declarations that the user needs to know about. Everything else should be hidden in the .tpp
Your code is quite verbose and has some potential for refactoring. For example, repetitive tasks make a great case for extracting a function and increasing code readability. Your feeling for this will grow quickly 🙂
You're not only testing the final check_isomorphism function, but also all the steps in between. This a reasonable for debugging, but might limit refactoring opportunities down the road if you're doing test-driven development

Let me know if there's any additional support I can provide!🙂 Looking forward to your next commits!

joweich

@sracha4355 I added a few more prescriptive comments to the code to give you a more tangible idea of the potentials for improvement mentioned above 🙂

joweich · 2023-10-25T15:14:11Z

include/graaflib/algorithm/graph_isomorphism.h

+
+namespace graaf::algorithm {
+
+// Type aliases


Following naming conventions, you'd start your alias names with a lowercase letter. You could also make Mapping a bit more descriptive, e.g. vertex_mapping.

joweich · 2023-10-25T15:14:21Z

include/graaflib/algorithm/graph_isomorphism.h

+ */
+template <typename V, typename E, graph_type T>
+std::optional<Mapping>
+check_isomorphism(


This is the only function users are supposed to call, correct? All the other definitions should be hidden in the .tpp file then.

Do the other functions that will not be called by the user need comments explaining how they work?

They don't if the code is self-explanatory. They can also be added later if we see the need

joweich · 2023-10-25T15:14:43Z

include/graaflib/algorithm/graph_isomorphism.h

+ */
+template <typename V, typename E, graph_type T>
+vf2_state * init_vf2_state(
+	const graph <V, E, T> & lhs,


Suggested change

const graph <V, E, T> & lhs,

const graph <V, E, T>& lhs,

The convention in this project is dropping the whitespace in these cases

joweich · 2023-10-25T15:15:43Z

include/graaflib/algorithm/graph_isomorphism.tpp

+		return true;
+
+	Candidate_pairs candid_pairs = generate_candidate_pairs(lhs, rhs , *state);
+	for(int i = 0; i < candid_pairs.size(); i++){	


Suggested change

for(int i = 0; i < candid_pairs.size(); i++){

for(const auto [first, second] : candid_pairs){

Range-iteration makes your code more concise. You can then reference first and second directly without the need of an iterator. This also applies to other parts of your PR

joweich · 2023-10-25T15:16:04Z

include/graaflib/algorithm/graph_isomorphism.tpp

+
+
+template <typename V, typename E, graph_type T>
+void update_terminal_sets(


This function essentially does the same exercise 4 times. We should extract a function that is called 4 times here to be more concise.

joweich · 2023-10-25T15:16:20Z

include/graaflib/algorithm/graph_isomorphism.tpp

+}
+
+template <typename V, typename E, graph_type T>
+void update_mappings(


Same as above. The same exercise is done 4 times, which should be extracted into a function. This also applies to a few subsequent functions

joweich · 2023-10-25T15:16:34Z

include/graaflib/algorithm/graph_isomorphism.tpp

+
+
+template <typename V, typename E, graph_type T>
+bool feasibility(


The naming should be more expressive here. What is checked for feasibility?

sracha4355 · 2023-10-25T16:44:31Z

Hey @sracha4355, huge congrats on your very first outside-of-school C++ PR 🚀 You definitely chose a challenging one for this and did very well! Some early guardrails for increasing compliance with best practices and making your code more concise:

In the header file, you should only have the declarations that the user needs to know about. Everything else should be hidden in the .tpp

Your code is quite verbose and has some potential for refactoring. For example, repetitive tasks make a great case for extracting a function and increasing code readability. Your feeling for this will grow quickly 🙂

You're not only testing the final check_isomorphism function, but also all the steps in between. This a reasonable for debugging, but might limit refactoring opportunities down the road if you're doing test-driven development

Let me know if there's any additional support I can provide!🙂 Looking forward to your next commits!

Thanks @joweich, do you recommend removing the tests related to the helper functions? I agree that it was helpful for debugging. Sure, I can definitely refactor the code. For comments, how descriptive should they be, should I try to explain every aspect of how and why the algorithm works this way? Is it mandatory to also add comments to my tests?

joweich · 2023-10-26T14:45:14Z

For comments, how descriptive should they be, should I try to explain every aspect of how and why the algorithm works this way? Is it mandatory to also add comments to my tests?

@sracha4355 the best code is code that does not need comments to be comprehensive for readers. In other words, if your code is self-explanatory, there's no need for any comments (apart from the interface documentation with the @-signs)

sracha4355 · 2023-10-30T22:01:19Z

@joweich is the .tpp not subject to clang formatting? How can I check if my new changes follow clang format. Is there a tool to automatically format the code for me

joweich · 2023-10-31T06:01:09Z

@joweich is the .tpp not subject to clang formatting?

Yes, it is. Codestyle concerns every sourcefile 🙂

How can I check if my new changes follow clang format. Is there a tool to automatically format the code for me

Check this guide for using clang format. Most IDEs have inbuilt support for this

Satvik Racharla added 4 commits September 4, 2023 12:41

typo in graph.h on a comment about method description, I am assuming …

88e416f

…it was supposed to say internal

working version of the algorithm

961c455

error in interpreting the core sets for the final mapping, fixed it

51c20a2

added comments to the header file, need to finish adding comments for…

ac10f06

… the test and .tpp file

github-actions bot reviewed Oct 25, 2023

View reviewed changes

sracha4355 changed the title ~~Requesting a code review of the VF2 Algorithm~~ Requesting a code review of the VF2 Algorithm for ISSUE: 87 Oct 25, 2023

joweich linked an issue Oct 25, 2023 that may be closed by this pull request

[ALGO] Graph Isomorphism #87

Closed

6 tasks

joweich reviewed Oct 25, 2023

View reviewed changes

sracha4355 closed this Nov 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Requesting a code review of the VF2 Algorithm for ISSUE: 87 #168

Requesting a code review of the VF2 Algorithm for ISSUE: 87 #168

sracha4355 commented Oct 25, 2023

github-actions bot left a comment

codecov bot commented Oct 25, 2023

joweich left a comment •

edited

Loading

joweich left a comment

joweich Oct 25, 2023

joweich Oct 25, 2023

sracha4355 Oct 25, 2023

joweich Oct 26, 2023

joweich Oct 25, 2023

sracha4355 Oct 25, 2023

joweich Oct 25, 2023

joweich Oct 25, 2023

joweich Oct 25, 2023

joweich Oct 25, 2023

sracha4355 commented Oct 25, 2023

joweich commented Oct 26, 2023

sracha4355 commented Oct 30, 2023

joweich commented Oct 31, 2023

	for(int i = 0; i < candid_pairs.size(); i++){
	for(const auto [first, second] : candid_pairs){



		template <typename V, typename E, graph_type T>
		void update_terminal_sets(



		template <typename V, typename E, graph_type T>
		bool feasibility(

Requesting a code review of the VF2 Algorithm for ISSUE: 87 #168

Requesting a code review of the VF2 Algorithm for ISSUE: 87 #168

Conversation

sracha4355 commented Oct 25, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2023

Codecov Report

joweich left a comment • edited Loading

Choose a reason for hiding this comment

joweich 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

sracha4355 commented Oct 25, 2023

joweich commented Oct 26, 2023

sracha4355 commented Oct 30, 2023

joweich commented Oct 31, 2023

joweich left a comment •

edited

Loading