Adds MaximalCommonSubdigraph and MinimalCommonSupergraph #334
Conversation
|
don't merge, something weird is happening with the commits |
le27
force-pushed
the
minmax
branch
3 times, most recently
from
3751e56
to
0507ff4
Compare
|
I think I fixed the problem I mentioned in my previous message |
There was a problem hiding this comment.
I'm afraid I got lost trying to figure out the mathematical details of the implementation, but from what I can see this looks great, and it works really well! I have asked two questions in my review, apologies if they are a bit naive, I may have overlooked the reasons you chose to do things otherwise.
Also, I don't know whether you wanted to maybe add some documentation for these two new functions; looks like they'd fit quite well in either section 3.3, or maybe 7.2 or 7.3.
gap/grahom.gi
Outdated
| Clqus := DigraphMaximalCliquesReps(B); | ||
|
|
||
| if Clqus = [] then | ||
| return [Digraph([]), [], []]; |
There was a problem hiding this comment.
Would there potentially be an interest in getting the second and third list elements to be transformations in this case? I'm not sure what you think of returning IdentityTransformation rather than [] if the maximal common subdigraph has no vertices; but that way we can guarantee that this function will always output a list which has transformations as second and third elements. For example,
gap> comm := MaximalCommonSubdigraph(D1, D2);;
gap> 2 ^ comm[2];will never return any errors.
There was a problem hiding this comment.
I agree, I'll fix this this thanks.
| gap> MaximalCommonSubdigraph(CompleteDigraph(100), CompleteDigraph(100)); | ||
| [ <immutable digraph with 100 vertices, 9900 edges>, IdentityTransformation, | ||
| IdentityTransformation ] | ||
| gap> MinimalCommonSuperdigraph(CompleteDigraph(100), CompleteDigraph(100)); |
There was a problem hiding this comment.
To be even more certain that these functions continue working in the future, perhaps there should be some tests that check not only equality of display, but also equality of objects, here? For example:
gap> comm := MinimalCommonSuperdigraph(CompleteDigraph(100), CompleteDigraph(100));;
gap> comm[1] = CompleteDigraph(100);
trueI'm not sure to what extent this is implemented in other test files, it may be overkill :)
gap/grahom.gi
Outdated
| out1 := OutNeighbours(D1); | ||
| out2 := OutNeighbours(D2); | ||
|
|
||
| adj := function(pair) |
There was a problem hiding this comment.
I'm pretty sure that we've discussed this already, but I think you could improve the performance here, by:
- Not using
Cartesianbut instead doing:
L := List([1 .. Size(out1) * Size(out2)], x -> []);
for i in [1 .. Size(out1)] do
for j in [1 .. Size(out2)] do
if isadj(i, j) then
Add(L[i], j);
fi;
od;
od;
Also the function isadj could be improved, by to run through the out neighbours of D1 and D2 once by using the adjacency matrix of D1 and D2
There was a problem hiding this comment.
@le27 this is awesome, but you still need to write the documentation please!
| edges := List([1 .. n * m], x -> []); | ||
| for i in [0 .. (n * m * n * m - 1)] do | ||
| t := intto4tuple(i); | ||
| # not that we are only concerned with cliques so we can ignore edges |
There was a problem hiding this comment.
| # not that we are only concerned with cliques so we can ignore edges | |
| # note that we are only concerned with cliques so we can ignore edges |
| end; | ||
|
|
||
| # As we are only concerned with cliques we don't need to consider the isolated | ||
| # vertices of MPG so we constrict it without them |
There was a problem hiding this comment.
| # vertices of MPG so we constrict it without them | |
| # vertices of MPG so we construct it without them |
| if vertices = [] then | ||
| for i in DigraphVertices(D1) do | ||
| for j in DigraphVertices(D2) do | ||
| if AdjacencyMatrix(D1)[i][i] = AdjacencyMatrix(D2)[j][j] then |
There was a problem hiding this comment.
You assign the variables A1 and A2 to AdjacencyMatrix(D1) and AdjacencyMatrix(D2) above, so why not use these variables again here?
| # we represent an element of V(D1)xV(D2)xV(D1)xV(D2) as an element of | ||
| # [0 .. (n x m x n x m) - 1] | ||
| intto4tuple := function(i) | ||
| local t, tempi; |
There was a problem hiding this comment.
This could be more compact as:
intto4tuple := i -> [RemInt(i, n),
QuoInt(RemInt(i, n * m), n),
QuoInt(RemInt(i, n * m * n), n * m),
QuoInt(i, n * m * n)];
There was a problem hiding this comment.
But with things properly aligned...
| D1 := DigraphImmutableCopy(A); | ||
| D2 := DigraphImmutableCopy(B); | ||
|
|
||
| # If the digraphs are isomorphic then we return the first one as the answer |
There was a problem hiding this comment.
Shouldn't the isomorphism check be done before A and B are copied? After all what's the point of copying them if it's not necessary.
|
Superseded by #361 |
The function MaximalCommonSubdigraph takes as input 2 digraphs and returns as output a digraph of maximum size which embeds into both of them as an induced subdigraph, as well embeddings of it into the given digraphs. The embeddings are given as transformations.
The function MinimalCommonSuperdigraph takes as input 2 digraphs and returns as output a digraph of minimum size which embeds into both into which they both embed as induced subdigraphs, as well embeddings of the given digraphs into it. The embeddings are again given as transformations.