Graph isomorphism and canonization via NautyGraphs.jl #418
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #418 +/- ##
==========================================
- Coverage 97.31% 96.57% -0.74%
==========================================
Files 117 120 +3
Lines 6956 7016 +60
==========================================
+ Hits 6769 6776 +7
- Misses 187 240 +53 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for setting this up! I can hopefully help with the first couple of rounds of review before this gets to the core maintainers.
This is pretty close to what I have in mind, I really appreciate you put in the time to prepare it. I think my main suggestion is that this can be a bit simpler if we remove most of the MethodError and error calls and just rely on Julia to raise the correct MethodError (that we improve with registering hints). I left a few more detailed comments below, but if something is unclear I would be happy to prototype a few more specific examples.
I put in a few suggestions for rewording the docs, but they are just personal preferences.
| function Graphs.Experimental.has_induced_subgraphisomorph( | ||
| g1::AbstractGraph, | ||
| g2::AbstractGraph, | ||
| ::AlgNautyGraphs; | ||
| vertex_relation::Union{Nothing,Function}=nothing, | ||
| edge_relation::Union{Nothing,Function}=nothing, | ||
| )::Bool | ||
| error( | ||
| "Induced subgraph isomorphims are currently not supported by `NautyGraphs`. Please use a different isomorphism algorithm.", | ||
| ) | ||
| return nothing | ||
| end |
There was a problem hiding this comment.
I do not think it is necessary to write these methods. This should just be a MethodError, no need to raise your own errors.
| return has_induced_subgraphisomorph( | ||
| g1, g2, alg; vertex_relation=vertex_relation, edge_relation=edge_relation | ||
| ) | ||
| throw(MethodError(has_induced_subgraphisomorph, (g1, g2, alg))) |
There was a problem hiding this comment.
This seems like a breaking change to me. Why is it needed?
There was a problem hiding this comment.
also, generally, I try to avoid throwing my own MethodErrors. Structuring the dispatch such that julia is the one the throw MethodErrors usually leads to simpler code (and then error hints can be used for more messages).
| - `alg`: The algorithm that is used to find the induced subgraph isomorphism. Can be only | ||
| `VF2()` at the moment. | ||
| - `alg`: The algorithm that is used to find the induced subgraph isomorphism. Can be | ||
| `VF2()` or `AlgNautyGraphs()`, if `NautyGraphs` is installed and imported. |
There was a problem hiding this comment.
| `VF2()` or `AlgNautyGraphs()`, if `NautyGraphs` is installed and imported. | |
| `VF2()` or, if `NautyGraphs` is imported, `AlgNautyGraphs()`. |
| """ | ||
| AlgNautyGraphs | ||
|
|
||
| An empty concrete type used to dispatch to [`NautyGraphs`](@ref) isomorphism functions. |
There was a problem hiding this comment.
| An empty concrete type used to dispatch to [`NautyGraphs`](@ref) isomorphism functions. | |
| A "configuration" type used to dispatch to isomorphism algorithms from the `nauty_graphs` C library, | |
| wrapped by the `NautyGraphs.jl` package. |
|
|
||
| Permute the vertices of graph `g` in-place, according to permutation `p`. No checking is done to verify that p is a permutation. | ||
| """ | ||
| function permute! end |
There was a problem hiding this comment.
this should probably not be a new function, just a new method for the existing Base.permute! function
| function canonize!(g::AbstractGraph, alg::IsomorphismAlgorithm=AlgNautyGraphs()) | ||
| throw(MethodError(canonize!, (g, alg))) | ||
| end |
There was a problem hiding this comment.
this can be function canonize! end so that we do not manually throw MethodError. Then we can have a hint that detects the method error and warns that one needs to import NautyGraphs.
| at this moment, which requires `NautyGraphs` to be installed and imported. | ||
|
|
||
| ### Examples | ||
| ```doctest.jl |
There was a problem hiding this comment.
huh... I did not know that you can also use doctest.jl as a specifier for doctest!
|
NautyGraphs.jl is interesting, but before you put too much effort into this PR, I don't see what we gain by making an extension in Graphs.jl? NautyGraphs.jl depends on Graphs.jl, so we can just import the symbols there and create concrete implementations there? |
You are right about not needing the extension itself, that is my fault for misleading @mxhbl . I did not catch that the dependency already existed, rather silly of me. Apologies for that! Either way, three points to specify though:
|
Following the suggestion of @Krastanov, this PR adds NautyGraphs.jl as a weakdep, which makes it possible to use nauty for graph isomorphism and canonization.
Main Changes
Graphs.Experimental.canonize!(g, alg)that permutes a graphs vertices into the canonical order defined by the algorithmalg. If two graphsg1andg2are isomorphic,g1 == g2after canonization.NautyGraphsExtpackage extension, which implementscanonize!andhas_isomorphwith NautyGraphs as a backend. Other isomorphism algorithms are currently not supported, and throw an error if called withalg=AlgNautyGraphs(). Vertex and edge relations are currently also not supported, but could in principle be implemented by cleverly transforming the input graph.canonize!permutes a graph's vertices in-place, I also implemented a functionpermute!(g::Graph, p)that performs the permutation.Other Changes
Graphs.Experimentalwith an algorithm that doesn't define an implementation method results in a stack overflow. Since the dispatch structAlgNautyGraphsneeds to be define inside the main Graphs module and not in the package extension, this would cause stack overflow errors if the user tries to call e.g.has_isomorph(g, h, AlgNautyGraphs())without loading NautyGraphs first. I changed this to manually throw method errors in the fallback methods, which leads to nicer error messages for the user.Tests?
I am not sure how to best test the isomorphism and canonization, both in Graphs.jl, but also in NautyGraphs.jl itself. Are there any standard test sets of graphs that are not too hard to check but also nontrivial? Should I test NautyGraphs against VF2?