Skip to content
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

Make isomorphism algorithm generic and add subgraph isomorphism check #369

Merged
merged 20 commits into from
Oct 14, 2020
Merged

Make isomorphism algorithm generic and add subgraph isomorphism check #369

merged 20 commits into from
Oct 14, 2020

Conversation

althonos
Copy link
Contributor

@althonos althonos commented Aug 25, 2020

Hi there!

I refactored the code in isomorphism.rs to make the algorithms generic, and I also added functions to check for subgraph isomorphism as requested in #48.

Added

  • petgraph::visit::EdgeCount trait to get the edge count for a graph (which is needed to check for the edge count before actually the VF2 matching algorithm), which is implemented for all graph implementations.
  • petgraph::algo::is_isomorphic_subgraph that checks if a graph is isomorphic to a subgraph of another graph (and petgraph::algo::is_isomorphic_subgraph_matching for the semantic matching variant)

Changed

  • petgraph::algo::is_isomorphic is now generic over its two input graphs, meaning that they don't even have to be of the same concrete type. There is however a check at compile time to make sure both graphs are either Directed or Undirected.
  • Semantic checking can be performed even if both graphs don't have the same data type, as long as the node_match and edge_match callbacks are properly typed.
  • The private code of is_isomorphic is now properly encapsulated, whereas the old code was doing everything in one big function with inline closures.
  • I inlined some implementations of NodeIndexable trait as not doing so had an impact on performance (the generic code uses to_index and from_index methods much more)

Benchmarks

graph.bench are benchmarks with the code from the master branch, generic.bench with the code from this PR.

>>> cargo benchcmp graph.bench generic.bench 
 name                       graph.bench ns/iter  generic.bench ns/iter  diff ns/iter  diff %  speedup 
 full_iso_bench             1,582                1,549                           -33  -2.09%   x 1.02 
 petersen_iso_bench         1,277                1,299                            22   1.72%   x 0.98 
 petersen_undir_iso_bench   913                  898                             -15  -1.64%   x 1.02 
 praust_dir_no_iso_bench    1,102,471            1,120,120                    17,649   1.60%   x 0.98 
 praust_undir_no_iso_bench  1,052,307            1,047,779                    -4,528  -0.43%   x 1.00

To do

  • Figure out a way for is_isomorphic not to require the input to be DataMap (probably by changing the SemanticMatcher trait).
  • Figure out a way to perform semantic feasibility validation for edges in a generic manner: currently the code needs to access an edge given its two endpoints, (this is causing the iso_matching test to fail currently)
  • Figure out a way for is_isomorphic not to require IntoEdgesDirected for the semantic validation of edges (IntoNeighborsDirected only is required for the rest of the algorithm).
  • Add a benchmark to assess the performance of semantic-checking isomorphism functions.

@althonos althonos marked this pull request as draft August 25, 2020 10:55
@althonos althonos changed the title [WIP] Make isomorphism algorithm generic and add subgraph isomorphism check Make isomorphism algorithm generic and add subgraph isomorphism check Aug 25, 2020
@althonos althonos marked this pull request as ready for review August 25, 2020 19:39
@XVilka
Copy link
Member

XVilka commented Aug 26, 2020

Wow, amazing. On the quick look it is good, but I need more time to read it thoroughly and test. Good job!

@XVilka XVilka added this to the 0.6 milestone Aug 26, 2020
@XVilka XVilka mentioned this pull request Sep 11, 2020
@althonos
Copy link
Contributor Author

althonos commented Oct 5, 2020

@XVilka ping me if you need me to rebase / change anything 😉

@XVilka
Copy link
Member

XVilka commented Oct 14, 2020

Please next time don't include whitespace changes - it makes review process harder, especially with those huge graph maps of 1 and 0.

@XVilka XVilka merged commit 92d5c11 into petgraph:master Oct 14, 2020
@althonos
Copy link
Contributor Author

Please next time don't include whitespace changes - it makes review process harder, especially with those huge graph maps of 1 and 0.

Sorry about that, my editor removes trailing whitespaces and I always notice it too late.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants