Closed jaanos closed 8 years ago
Branch: u/jaanos/add_is_cayley_graph
Description changed:
---
+++
@@ -1 +1 @@
-
+This ticket adds a method `is_cayley_graph` to `GenericGraph`. This method checks whether a graph or digraph (which may or may not be connected) is a Cayley graph. If a certificate is requested, it also returns a permutation group and a generator set which generate this graph.
Author: Janoš Vidali
Changed keywords from none to Cayley graphs groups
I should point out some questions and issues I have with this before it goes into review.
is_cayley_graph
, or just is_cayley
?conjugacy_classes_subgroups
. Ticket #19585 already improves this, but it would be even better to do something like this:gap = A._gap_().parent()
C = gap.new("""
First(ConjugacyClassesSubgroups(%s),
x -> Order(Representative(x)) = %d
and IsTransitive(Representative(x), [1..%d]));
""" % (A._gap_().name(), n, n))
c = (gap.eval('%s = fail' % C.name()) != 'true')
if c and certificate:
G = A.subgroup(gap_group=C.Representative())
Now, I guess that maybe this doesn't really belong in generic_graph.py
. But maybe we could have a method in PermutationGroup_generic
which checks for the existence of such a group (and have it returned if requested)?
# -*- coding: utf-8 -*-
to the top of the file (haven't tested it), but I don't know if we want to do it:)New commits:
9fc9824 | Initial documentation for is_cayley_graph() |
86c766f | Implement is_cayley_graph for connected graphs |
60142d0 | Add tests for is_cayley_graph |
0f925f6 | Implement is_cayley_graph for disconnected graphs |
fbff6b8 | Add note regarding optional packages |
Hellooooo,
I should point out some questions and issues I have with this before it goes into review.
I will try t answer them.
- Should the method be called
is_cayley_graph
, or justis_cayley
?
We have 'is_perfect', 'is_chordal', 'is_cartesian_product', so I'd vote for 'is_cayley'.
Now, I guess that maybe this doesn't really belong in
generic_graph.py
. But maybe we could have a method inPermutationGroup_generic
which checks for the existence of such a group (and have it returned if requested)?
Do you know how such a function could be named? Having 'nice and natural name' for this function would rule out the alternative of having a 'hidden helper function' for this code.
- I should probably check whether the function works correctly with graphs with loops and multigraphs (my instinct says yes for the former and no for the latter).
You are not 'forced' to make it work in those cases. If it is complicated or if you do not want to deal with those cases, you should look at 'scream_if_not_simple'. It happens very often throughout the graph/ code.
- Can we have Unicode characters in the docstrings?
We can, and you can add this directive to the top of the file if you need to. Now, I am not really a big fan of adding a name every time one adds a function. That's just me, and perhaps some others. We already have 'git blame' to figure out who wrote a function, and having in each function the names of all people who ever touched it could quickly grow out of hands. And I personally don't see the added value, and I personally don't want to see Sage's source code turn out into a way to advertise its developers.
I added 'personally' everywhere to insist that it is my view, and that other developers have different opinions. By our customs, you are totally allowed to add your name in an author block if you want to.
Thanks for adding this function,
Nathann
Hi!
- Should the method be called
is_cayley_graph
, or justis_cayley
?We have 'is_perfect', 'is_chordal', 'is_cartesian_product', so I'd vote for 'is_cayley'.
OK, will change.
Now, I guess that maybe this doesn't really belong in
generic_graph.py
. But maybe we could have a method inPermutationGroup_generic
which checks for the existence of such a group (and have it returned if requested)?Do you know how such a function could be named? Having 'nice and natural name' for this function would rule out the alternative of having a 'hidden helper function' for this code.
Maybe has_transitive_subgroup(order = None, certificate = False)
? By default, it would just tell whether the group itself is transitive; otherwise, a subgroup of given order would be looked for (and returned if certificate
is True
).
- I should probably check whether the function works correctly with graphs with loops and multigraphs (my instinct says yes for the former and no for the latter).
You are not 'forced' to make it work in those cases. If it is complicated or if you do not want to deal with those cases, you should look at 'scream_if_not_simple'. It happens very often throughout the graph/ code.
I don't think it will be too complicated, I just haven't thought about it when coding. But thanks for the info, it is certainly useful to know about this option.
- Can we have Unicode characters in the docstrings?
We can, and you can add this directive to the top of the file if you need to. Now, I am not really a big fan of adding a name every time one adds a function. That's just me, and perhaps some others. We already have 'git blame' to figure out who wrote a function, and having in each function the names of all people who ever touched it could quickly grow out of hands. And I personally don't see the added value, and I personally don't want to see Sage's source code turn out into a way to advertise its developers.
I added 'personally' everywhere to insist that it is my view, and that other developers have different opinions. By our customs, you are totally allowed to add your name in an author block if you want to.
OK, I will remove my name (indeed, I had copied the docstring of one of your functions, so the author field was there) - I am certainly not claiming to have invented this algorithm:)
Thanks for adding this function,
No problem!
Janoš
Helloooooo,
Maybe
has_transitive_subgroup(order = None, certificate = False)
? By default, it would just tell whether the group itself is transitive; otherwise, a subgroup of given order would be looked for (and returned ifcertificate
isTrue
).
+1 to that.
OK, I will remove my name (indeed, I had copied the docstring of one of your functions, so the author field was there) - I am certainly not claiming to have invented this algorithm:)
Oh. Well, then it probably sounds very hypocritical of me to say that when you copy/pasted a function which contained my own name in an author field O_o
I cannot tell which function it was, but I really do try to keep this to a minimum (at least when it comes to my own name).
Nathann
Branch pushed to git repo; I updated commit sha1. New commits:
2315138 | Include fast GAP code |
e4e7142 | Merge branch 'is_cayley_graph' into is_cayley_graph-gap |
ddcf055 | Fix checking for existence of an appropriate subgroup |
857a989 | Merge branch 'is_cayley_graph' into is_cayley_graph-gap |
a92bc60 | Initial documentation for has_transitive_subgroup() |
3ee5be2 | Write method has_transitive_subgroup |
10b83ca | Rewrite is_cayley using has_transitive_subgroup |
Branch pushed to git repo; I updated commit sha1. New commits:
2e14f69 | Add examples for has_transitive_subgroup |
Description changed:
---
+++
@@ -1 +1,3 @@
-This ticket adds a method `is_cayley_graph` to `GenericGraph`. This method checks whether a graph or digraph (which may or may not be connected) is a Cayley graph. If a certificate is requested, it also returns a permutation group and a generator set which generate this graph.
+This ticket adds a method `is_cayley` to `GenericGraph`. This method checks whether a graph or digraph (which may or may not be connected) is a Cayley graph. If a certificate is requested, it also returns a permutation group and a generator set which generate this graph.
+
+A method `has_transitive_subgroup` has also been added to `PermutationGroup_generic`. For a given order, it determines whether the group has a subgroup of this order, and returns it if requested. If no order is given, it is taken to be the order of the group itself (i.e., the method then simply tells whether the group is transitive).
OK, I think I have addressed all the issues. If this is OK, I guess we can also make more direct calls to GAP in the methods conjugacy_classes_subgroups
and subgroups
(in another ticket, of course).
Dima, I add you to this ticket just in case you might have some time to look at PermutationGroup.has_transitive_subgroup
.
Hello Janoš,
I wonder about the 'is_connected' case in your algorithm. Does it really make a different in the running times? I thought that perhaps the 'isomorphism_group' implementations already handled that, in which cas it may not make any significant difference and so we could do without this specific block of code?
Nathann
Hi!
Testing with the example in the docstring (5*Paley(9)) confirms that there is a big difference in running times. The reason is of course that disconnected vertex-transitive graphs will have a very large group of automorphisms, and GAP will take very long to compute the lattice of subgroups. So, in most cases it will be cheaper to do the check for a connected component, and then inflate the certificate if requested (what the method actually does is return the direct product of the group for a connected component and a cyclic group).
Janoš
Oh, of course. Sorry for this question: I had first thought that you were optimizing the computation of automorphism_group
, but of course you were optimizing the call to conjugacy_classes_subgroups
.
adj = [y if v == x else x for x, y, z in self.edge_iterator(v)]
Are you looking for Graph.neighbors
?
Nathann
Hi!
Are you looking for
Graph.neighbors
?
I had this, but changed to edge_iterator
since neighbors
does not list a vertex multiple times if there are multiple edges to it (so maybe this is something that should be changed?). Also, this covers digraphs without needing an additional if
.
Janoš
Yo,
I am impressed with your code: not a single line seems out of place. Could you add a comment around this call to 'edge_iterator', to say that you mean to deal with all (out-)neighbors with multiplicity?
Could you also add a doctest (possibly in a 'TESTS:' section) for those directed/multigraph cases?
I believe that the graph part of your code is good to go, but I would like to have Dima's input for the 'gap'-specific code.
Thanks,
Nathann
Hi!
I am impressed with your code: not a single line seems out of place. Could you add a comment around this call to 'edge_iterator', to say that you mean to deal with all (out-)neighbors with multiplicity?
Thanks! Will do, just a bit later (currently I'm trying various solutions for #19585).
Could you also add a doctest (possibly in a 'TESTS:' section) for those directed/multigraph cases?
Sure. By the way, do we have any easily-constructible multigraphs/graphs with loops ready? Otherwise there'd be a few lines of code just to construct the graph.
Also, cayley_graph
on a group will not return multiedges or loops, even when the generators contain repeated elements or the identity and simple
is set to False
. So maybe this is something that should be changed.
Janoš
Hello,
Thanks! Will do, just a bit later (currently I'm trying various solutions for #19585).
Okayyyyyy.
Sure. By the way, do we have any easily-constructible multigraphs/graphs with loops ready? Otherwise there'd be a few lines of code just to construct the graph.
The only ones I know are the directed GNP:
sage: digraphs.RandomDirectedGNP(5,.2,loops=True)
RandomDirectedGNP(5,0.200000000000000): Looped digraph on 5 vertices
Also,
cayley_graph
on a group will not return multiedges or loops, even when the generators contain repeated elements or the identity andsimple
is set toFalse
. So maybe this is something that should be changed.
If you have a use for something like that, of course.
Nathann
Branch pushed to git repo; I updated commit sha1. New commits:
95dfa64 | Add a doctest and some comments |
Hi!
OK, I added the comment and the doctest.
The only ones I know are the directed GNP:
Thanks, but of course these won't help here. I've used an augmented Paley(9) graph instead.
Also,
cayley_graph
on a group will not return multiedges or loops, even when the generators contain repeated elements or the identity andsimple
is set toFalse
. So maybe this is something that should be changed.If you have a use for something like that, of course.
Not really, except possibly for doctesting (and consistency).
Janoš
I'd investigate whether being more careful with computing subgroups might help. Perhaps you can use some lower level GAP calls to find a lattice of transitive subgroups, which ought to be much smaller.
Or you can compute maximal subgroup classes (cf. GAP's ConjugacyClassesMaximalSubgroups
), and recurse on these which are transitive.
Hello!
Thanks, will look into it. It seems that it could be done without too much trouble using MaximalSubgroupClassReps
.
Janoš
Hello again!
Sorry about the inactivity, I have been pretty busy. Also, I was probably too quick to judge that a more low-level GAP procedure could be done without too much trouble.
Would it be OK to accept this as is, and then possibly improve the search for transitive subgroups in another ticket?
Janoš
Sorry about the inactivity, I have been pretty busy. Also, I was probably too quick to judge that a more low-level GAP procedure could be done without too much trouble.
Would it be OK to accept this as is, and then possibly improve the search for transitive subgroups in another ticket?
No, all the code has to be reviewed. I expect it to be as clean as the part I reviewed, yet somebody has to do check it. I cannot tell about the code, but the name 'has_transitive_subgroup', for instance, sounds misleading. It does not mention any kind of 'order', so by just lookig at the function's name you would not understand how it could return anything different from .is_transitive
. That's also the kind of things a reviewer should look at, besides the GAP technicalities.
Nathann
Hi!
No, all the code has to be reviewed. I expect it to be as clean as the part I reviewed, yet somebody has to do check it. I cannot tell about the code, but the name 'has_transitive_subgroup', for instance, sounds misleading. It does not mention any kind of 'order', so by just lookig at the function's name you would not understand how it could return anything different from
.is_transitive
. That's also the kind of things a reviewer should look at, besides the GAP technicalities.
Yes, I understand it has to be reviewed - I was just saying that maybe we don't try to optimize the GAP code within this ticket (if that's acceptable, of course).
As for the function name, do you have any suggestions? Maybe something like has_transitive_subgroup_of_order
?
Janoš
Yes, I understand it has to be reviewed - I was just saying that maybe we don't try to optimize the GAP code within this ticket (if that's acceptable, of course).
Oh, I see. Well, then I cannot answer the question as I do nto understand what the gap code does.
As for the function name, do you have any suggestions? Maybe something like
has_transitive_subgroup_of_order
?
Nothing better than that, and the name is so ugly that it might as well be a hidden 'helper' function :-/
Isn't there a group-theoretic notion that would 'mean the same'? :-/
Nathann
Replying to @nathanncohen:
Yes, I understand it has to be reviewed - I was just saying that maybe we don't try to optimize the GAP code within this ticket (if that's acceptable, of course).
Oh, I see. Well, then I cannot answer the question as I do nto understand what the gap code does.
As for the function name, do you have any suggestions? Maybe something like
has_transitive_subgroup_of_order
?
How about naming it transitive_subgroup
, and simplifying the interface,so that it either returns a subgroup of order
n(the parameter) or
None`, and no pairs...
as well, I'd propose to remove certificate
business totally, and rename the main function caley_graph_group()
: it would return a group if one exists, or None
if there is no such group; indeed, the extra cost of returning a group is zero, compared to the overall complexity of the task at hand.
Hi!
Hmm, I would still like to have an is_cayley
method for graphs. But I agree about having a method transitive_subgroup
for permutation groups. Still, I'm a bit worried about cases when we have a large graph with a relatively small automorphism group (say, twice the order of the graph) - then finding an appropriate subgroup will be fast, but actually returning it (so a Sage subgroup can be built) may make a non-negligible contribution to the running time (since reading GAP output can be slow). So I propose:
transitive_subgroup
method, which by default returns a transitive subgroup of given order,transitive_subgroup
may be explicitly told not to return the group (by setting, say, return_group
to False
),_transitive_subgroup_gap
) which returns a GAP object containing the subgroup (we could make the result cached, so if one first asks whether a graph is Cayley and then wants to know the group, it needn't be recomputed).This way, transitive_subgroup
can call the helper function, and then avoid actually reading the GAP subgroup if it is told not to return it.
What do you think?
Janoš
Branch pushed to git repo; I updated commit sha1. New commits:
4aaf301 | Rename has_transitive_subgroup to transitive_subgroup and add helper function |
I have implemented the ideas from the previous comment. I have marked both transitive_subgroup
and _transitive_subgroup_gap
as cached, although currently it seems that this will not affect the (re)running times of is_cayley
, as the automorphism group of a graph is not cached. Is this something that should be changed (in another ticket, of course), or should I just remove caching for the two methods?
Replying to @jaanos:
Hmm, I would still like to have an
is_cayley
method for graphs.
sure, such a method can just check whether cayley_graph_group()
is None
.
But I agree about having a method
transitive_subgroup
for permutation groups. Still, I'm a bit > worried about cases when we have a large graph with a relatively small automorphism group (say, twice the order of the graph) - then finding an appropriate subgroup will be fast, but actually returning it (so a Sage subgroup can be built) may make a non-negligible contribution to the running time (since reading GAP output can be slow).
as a switch to libgap
will happen sooner or later, optimising for not reading GAP output
does not seem to be that important.
So I propose:
- we have a
transitive_subgroup
method, which by default returns a transitive subgroup of given order,transitive_subgroup
may be explicitly told not to return the group (by setting, say,return_group
toFalse
),- we add a helper method (say,
_transitive_subgroup_gap
) which returns a GAP object containing the subgroup (we could make the result cached, so if one first asks whether a graph is Cayley and then wants to know the group, it needn't be recomputed).This way,
transitive_subgroup
can call the helper function, and then avoid actually reading the GAP subgroup if it is told not to return it.What do you think?
this ceritainly looks better, but again, I don't like the counter-intuitive parameters of is_cayley()
and weird sort of output (a pair) that you currently have. In particular the latter.
Hi!
as a switch to
libgap
will happen sooner or later, optimising for not reading GAP output does not seem to be that important.
I see. But should I do anything about it at this point?
this ceritainly looks better, but again, I don't like the counter-intuitive parameters of
is_cayley()
and weird sort of output (a pair) that you currently have. In particular the latter.
I have modelled both input and output of is_cayley
after methods such as is_chordal
and is_circulant
. I guess we could be even more specific and have the function return only the group or only the generators if requested - or maybe cayley_graph_group
could do that, and is_cayley
would then simply call cayley_graph_group
without requesting either.
Janoš
Replying to @jaanos:
Hi!
as a switch to
libgap
will happen sooner or later, optimising for not reading GAP output does not seem to be that important.I see. But should I do anything about it at this point?
this ceritainly looks better, but again, I don't like the counter-intuitive parameters of
is_cayley()
and weird sort of output (a pair) that you currently have. In particular the latter.I have modelled both input and output of
is_cayley
after methods such asis_chordal
andis_circulant
.
not all silly designs in Sage should be copied...
Looks like someone didn't know, or forgot, about None
I guess we could be even more specific and have the function return only the group or only the generators if requested - or maybe
cayley_graph_group
could do that,
only the generators? Why? it's one quick function call away from the group, why would you do this in library code? In particular, as you are computing the group, not its generators, in the first place.
and
is_cayley
would then simply callcayley_graph_group
without requesting either.e.
sure.
Hello!
not all silly designs in Sage should be copied... Looks like someone didn't know, or forgot, about
None
I don't know, it seems pretty reasonable to me to have an is_property
method with the option to return a certificate for either having or not having this property. Now, I don't know of any way to provide an easy-to-check certificate of non-Cayleyness, but if one is eventually found, it can be returned alongside the boolean telling whether a graph is a Cayley graph - so the user doesn't need to check what kind of certificate the function returned.
As for the certificate of Cayleyness, I am thinking that a better option would be to return a map from vertices to group elements (which the current function already computes when computing the generators) - now it can be easily checked that the elements form a group, and that the generating set is well defined (instead of having to compute an isomorphism between the graph and the Cayley graph built from the group and generating set). Actually computing the group (and returning it alongside the generating set and the map, if requested) would then be delegated to cayley_graph_group
, as you proposed.
only the generators? Why? it's one quick function call away from the group, why would you do this in library code? In particular, as you are computing the group, not its generators, in the first place.
Well, I think if we're giving the user the choice of what they want returned, it would be unreasonable to forbid a certain choice just because we see no use for it.
Janoš
Replying to @jaanos:
only the generators? Why? it's one quick function call away from the group, why would you do this in library code? In particular, as you are computing the group, not its generators, in the first place.
Well, I think if we're giving the user the choice of what they want returned, it would be unreasonable to forbid a certain choice just because we see no use for it.
it's an unreasonable code bloat to include a special function for anything that is a trivial one-liner; e.g. if you start returning the generators, you might be tempted to add a function that only returns the number of generators, etc etc :-)
And you are not depriving the user from getting what he wants in any way by not providing an option for any trivial one-liner applied to the return value, not at all..
IMHO it's also against the spirit of OOP to have an option in a function f
to return either X
or X.blah()
; the user can call .blah()
on the answer he gets himself; and X
is already created in f
, there is no performance gain in returning X.blah() instead of X
.
IMHO it's also against the spirit of OOP to have an option in a function
f
to return eitherX
orX.blah()
; the user can call.blah()
on the answer he gets himself; andX
is already created inf
, there is no performance gain in returning X.blah() instead ofX
.
Are we sure that a permuation group defined from a set of generators returns the very same set of generators when you call .gen()
? Or could they be simplified/reduced if Gap believe that another representation is better?
Nathann
Replying to @nathanncohen:
IMHO it's also against the spirit of OOP to have an option in a function
f
to return eitherX
orX.blah()
; the user can call.blah()
on the answer he gets himself; andX
is already created inf
, there is no performance gain in returning X.blah() instead ofX
.Are we sure that a permuation group defined from a set of generators returns the very same set of generators when you call
.gen()
? Or could they be simplified/reduced if Gap believe that another representation is better?
In GAP GeneratorsOfGroup
need not return the original list of generators used to define the group in the first place.
39.2-2 GroupByGenerators
‣ GroupByGenerators( gens ) ──────────────────────────────────────────────────────────────── operation
‣ GroupByGenerators( gens, id ) ──────────────────────────────────────────────────────────── operation
GroupByGenerators returns the group G generated by the list gens. If a second argument id is present
then this is stored as the identity element of the group.
The value of the attribute GeneratorsOfGroup (39.2-4) of G need not be equal to gens.
GroupByGenerators is the underlying operation called by Group (39.2-1).
I don't know how to recover the original generators from Group
; it could very well be that they get lost, by design.
If one needs to keep the original ones, he can use GroupWithGenerators
.
Perhaps I was not being precise enough: I was talking about the generating set of the Cayley graph, not about the generators of the group.
So, a user may want any of the following (where computing each relies on the previous):
Now, going from step 2 to steps 3 and 4 surely is not hard (4 lines of code), but it is not something that can be done with a simple method call, and step 4 it is certainly not something that can be computed from the group itself.
Anyway, I was probably wrong that the map would be a better certificate than the group, as the former is clearly easily computable from the latter.
By the way, what we're actually looking for here is a regular subgroup, so that's what the group method should be called (and we can also simplify the GAP call).
Janoš
Replying to @jaanos:
Perhaps I was not being precise enough: I was talking about the generating set of the Cayley graph, not about the generators of the group.
well, how about having a proper digraphs.CayleyGraph()
constructor?
(one can use GRAPE package of GAP, which is going to be re-released under GPL)
e.g.
gap> LoadPackage("grape");
true
gap> C:=CayleyGraph(SymmetricGroup(4),[(1,2),(2,3),(3,4)]);
rec( adjacencies := [ [ 2, 3, 7 ] ], group := Group([ (1,10,17,19)(2,9,18,20)(3,12,14,21)(4,11,13,22)
(5,7,16,23)(6,8,15,24), (1,7)(2,8)(3,9)(4,10)(5,11)(6,12)(13,15)(14,16)(17,18)(19,21)(20,22)
(23,24) ]), isGraph := true, isSimple := true,
names := [ (), (3,4), (2,3), (2,3,4), (2,4,3), (2,4), (1,2), (1,2)(3,4), (1,2,3), (1,2,3,4),
(1,2,4,3), (1,2,4), (1,3,2), (1,3,4,2), (1,3), (1,3,4), (1,3)(2,4), (1,3,2,4), (1,4,3,2),
(1,4,2), (1,4,3), (1,4), (1,4,2,3), (1,4)(2,3) ], order := 24, representatives := [ 1 ],
schreierVector := [ -1, 1, 1, 2, 2, 1, 2, 2, 2, 1, 1, 1, 1, 2, 2, 1, 1, 2, 1, 1, 2, 2, 1, 2 ] )
gap>
This ticket adds a method
is_cayley
toGenericGraph
. This method checks whether a graph or digraph (which may or may not be connected) is a Cayley graph. If requested, it also returns a permutation group, a mapping from vertices to group elements, and a generating set of the Cayley graph.A method
has_regular_subgroup
has also been added toPermutationGroup_generic
. It determines whether the group has a regular subgroup, and returns it if requested.CC: @nathanncohen @dimpase
Component: graph theory
Keywords: Cayley graphs groups
Author: Janoš Vidali
Branch:
c6680e7
Reviewer: Nathann Cohen
Issue created by migration from https://trac.sagemath.org/ticket/19586