-
Notifications
You must be signed in to change notification settings - Fork 41
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
ISSUE 87: vf2 algorithm refactor #185
Conversation
…it was supposed to say internal
… the test and .tpp file
…er, now there are bugs within the newly refactored code which I will be addressing
…suppiled to vf2 that have no chance of being isomorphic will cause the algo to run till its end, which can be a long time given the size of the graphs
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
==========================================
- Coverage 99.59% 99.43% -0.16%
==========================================
Files 56 60 +4
Lines 2738 3390 +652
==========================================
+ Hits 2727 3371 +644
- Misses 11 19 +8 ☔ 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.
Hey @sracha4355, very great to see that you built on the feedback from last time (#172 (review))! 🚀
I added a few more comments that are mostly easy fixes. However, I haven't had an in-depth look into the details of graph_isomorphism_utils.tpp
. It's still a bit hard to follow what happens in the 500+ lines of code in this file. Given that this algorithm is quite complex, I feel you don't need to optimize/refactor every detail in this PR. Just stop refactoring once you feel you're at a dead end 🙂 We can then do more optimizations in a follow-up PR.
To finally get you your well-deserved contribution to the main branch, I'd suggest to have a look into my latest feedback, add some more tests for minimal and more complex cases, and remove the debugging statements and commented-out code.
At some point, we'll also need a documentation page for the VF2 algorithm, but this can be done in a follow-up PR as well 🙂
Marking this PR as stale. It will not be automatically closed. Even though the maintainers of Graaf may not always have time to take a look in a timely fashion, your contributions are much appreciated. |
Hi @sracha4355, sorry for the late reply and thanks a lot for working on this. The implementation of the VF2 algorithm is definitely a lot more complex than I initially thought 😅 Nonetheless, very nice to see that you managed to implement this from scratch! Before merging this though, I would like to gain some more confidence in the correctness of the implementation by enhancing the unit tests. Going forward, I would propose to close the current PR and to keep the branch as a reference implementation. Please feel free to ping me / re-open though if you would like to continue on this. |
@bobluppes yeah the algorithm is definitely a beast of its own. I will let you know if I am available to work on it! |
I've restructured the code by splitting up the responsibilities of the algorithm into multiple classes. There are three major components.
class vf2_target_sets handles 1 and 2
class vf2_vertex_id_mapper handles 3
class vf2_isomorphism_feasibility_checker handles 4
The struct vf2_information holds objects of classes vf2_target_sets and vf2_vertex_id_mapper.
The main function check_isomorphism uses this struct on its recursive calls and will pass along an object for class vf2_isomorphism_feasibility_checker. The code right now will not pass all the automated checks, but I need to know your thoughts on how I structured my code, the rest is an easy refactor. Thanks @joweich.