-
Notifications
You must be signed in to change notification settings - Fork 164
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
Return an iterator over all valid Vf2 mappings #376
Conversation
This commit adds a new function vf2_mapping which is used to get the isomorphic node mapping between 2 graphs. It works in the same way as is_isomorphic() but instead of simply returning a boolean whether the graphs are isomorphic it returns the node id mapping from first to second for the matching.
This commit fixes the construction of the reverse map when id_order=False. The previous versions of this did not correctly build the map in all cases. Co-authored-by: georgios-ts <45130028+georgios-ts@users.noreply.github.com>
Pull Request Test Coverage Report for Build 1136799811
💛 - Coveralls |
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.
This looks great! I especially like a lot of the internal refactoring of the the core vf2 algorithm to be more explicitly defined (ie functions on structs functions instead of a bunch of nested closures) and the iterator built on the reordered copy of the graph is an awesome way to workaround the lifetime issue. I've got a few inline comments but most are on small mechanical things.
Besides those the only other thing I think would be good to add test coverage for some of the uncovered negative paths from the coverage report. https://coveralls.io/builds/42102310/source?filename=src%2Fisomorphism%2Fvf2.rs or leave a comment why it's unreachable like you did for the paths which you did for some of those paths. Assuming it's feasible for some of those it looked like it'd be kind of annoying to come up with a test case to cause those conditions.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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.
LGTM, thanks for making the quick updates and adding more tests to catch the different negative cases.
Building on top of #368, this PR refactors Vf2 algorithm and implements a
vf2_mapping
function that returns a custom python class that acts as a python iterator that lazily evaluates all valid Vf2 mappings. The "trick" that allows the lazily evaluation is to take ownership of a copy of the graphs. We can do this (with minimal overhead) since we were already creating new graphs in case ofid_order=False
ornodes_removed=True
. At the same time, it adjustsVf2ppSorter
to handle graphs with non-contiguous node ids.TODO