-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Implement minimum_generating_set
function for group theory
#37481
base: develop
Are you sure you want to change the base?
Conversation
Although I have implemented the final working function but some changes are still pending like changing the variables names, and optimising the code to improve the performance.
I have added the documentation of the function, additionally, renamed some variables. And added a method to convert the final answer to sage. But it is failing for some cases. So need to fix those.
The method minimum_generating_set will be available for gap objects also, if we define it inside gapelement class but it might create confusion that whether that is a method of gap.
minimum generating set
for groupsminimum_generating_set
function for group theory
@tscrim, can you please review this PR? |
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.
I don't understand the gap_based
argument. At the end, everything you are doing only works for groups implemented in GAP. So I think you need to handle the case when a group does not have a working _libgap_
method more cleanly (e.g., Permutations(5)
). I also don't see the utility in returning GAP objects; I think it should always return elements of the corresponding Sage group. This would also simplify the code (this could also be done by the method as a sort of "post-processing", with the function returning GAP objects in case someone does find a use for it).
Also, check for PEP8 spacing.
Documentation preview for this PR (built with commit 047a071; changes) is ready! 🎉 |
@@ -451,6 +451,54 @@ def conjugacy_class(self, g): | |||
from sage.groups.conjugacy_classes import ConjugacyClass | |||
return ConjugacyClass(self, g) | |||
|
|||
def minimum_generating_set(self): | |||
""" | |||
Return a list of the minimum generating set of this group. |
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.
Return a list of the minimum generating set of this group. | |
Return a list of a minimum generating set of this group. |
Since it is not necessarily unique.
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.
What does "a list of a set" mean? Perhaps instead like this?
Return a list of the minimum generating set of this group. | |
Return a minimum generating set of this group. |
If you want to emphasize that it returns a list object and not a set object, I'd be explicit about that, e.g. "a min. gen. set, represent by a list of group elements" (though IMHO that's unnecessary)
src/sage/groups/libgap_mixin.py
Outdated
|
||
ALGORITHM: | ||
|
||
We follow the algorithm described in the research paper "A New Algorithm for Finding the Minimum Generating Set of a Group" by John Doe (:doi:`10.1016/j.jalgebra.2023.11.012`). |
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.
Please use a proper reference [Doe2023]_
, which your temp author should be replaced accordingly.
src/sage/groups/libgap_mixin.py
Outdated
1. If G is a cyclic group, it returns the cyclic generator. | ||
2. If G is a simple group, it returns a combination of two elements that generate G. | ||
|
||
If the above two cases fail, the algorithm finds the minimal normal subgroup N of G. | ||
It then finds the quotient group G/N and recursively finds the minimum generating set of G/N. | ||
Let S be the minimum generating set of G/N. The algorithm finds representatives g of S in G. | ||
|
||
If N is abelian, the algorithm checks if any case of the form g_1, g_2, ..., g_i*s_j, g_i+1, ..., g_lg | ||
(where lg is the length of g) is able to generate G. If not, it uses the set g_1, g_2, ..., g_lg, s_j | ||
as the minimum generating set of G. | ||
|
||
If N is non-abelian, the algorithm checks if any case of the form g_1*n_1, g_2*n_2, ..., g_lg*n_lg | ||
is able to generate G, where n_1, n_2, ..., n_lg can be any elements of N. If not, it checks if any | ||
case of the form g_1*n_1, g_2*n_2, ..., g_lg*n_lg, n_lg+1 is able to generate G. |
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.
Please use latex formatting for all of this, e.g.
`g_1, g_2, \ldots, g_n`
src/sage/groups/libgap_mixin.py
Outdated
for iter in product(N, repeat=lg): | ||
yield [g[i] * iter[lg-i-1] for i in range(lg)] |
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.
for iter in product(N, repeat=lg): | |
yield [g[i] * iter[lg-i-1] for i in range(lg)] | |
for it in product(N, repeat=lg): | |
yield [g[i] * it[lg-i-1] for i in range(lg)] |
Let's not use Python builtin functions as variable names.
src/sage/groups/libgap_mixin.py
Outdated
for i in range(n): | ||
for j in range(i+1, n): | ||
if is_group_by_gens(G, [group_elements[i], group_elements[j]]): | ||
return [group_elements[i], group_elements[j]] |
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.
for i in range(n): | |
for j in range(i+1, n): | |
if is_group_by_gens(G, [group_elements[i], group_elements[j]]): | |
return [group_elements[i], group_elements[j]] | |
from itertools import combinations | |
for a, b in combinations(group_elements, 2): | |
if is_group_by_gens(G, [a, b]): | |
return [a, b] |
This should be a bit faster of a check. At the very least, it is a bit shorter code.
src/sage/groups/libgap_mixin.py
Outdated
raise NotImplementedError("only implemented for finite groups") | ||
|
||
def is_group_by_gens(group, gens): | ||
return set(group.AsList()) == set(libgap.GroupByGenerators(gens).AsList()) |
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 seems awfully slow. At the very least, we should cache the set of elements in the group. It might also be good to check some easier to compute things first, such as the cardinalities, before checking all elements. Some timings should be done.
src/sage/groups/libgap_mixin.py
Outdated
if is_group_by_gens(G, [group_elements[i], group_elements[j]]): | ||
return [group_elements[i], group_elements[j]] | ||
|
||
N = G.MinimalNormalSubgroups()[0] |
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.
@RuchitJagodara, you should follow @fingolfin's advice. You should also profile your code to see where time is being spent. Right now, GAP's implementation is apparently completely beating yours, so it should be the preferred one unless you have data suggesting otherwise.
…gramming approach * Update groups.py * Update groups.py * Update libgap_mixin.py * Update libgap_mixin.py * Update groups.py * Update libgap_mixin.py * Update libgap_mixin.py * Update libgap_mixin.py * Update libgap_mixin.py * Update libgap_mixin.py * Update libgap_mixin.py * Update src/sage/groups/libgap_mixin.py Co-authored-by: Ruchit Jagodara <ruchit.jagodara@iitgn.ac.in> * Update src/sage/groups/libgap_mixin.py Co-authored-by: Ruchit Jagodara <ruchit.jagodara@iitgn.ac.in> * Update libgap_mixin.py * Update groups.py Removed the Example. * Update groups.py * Update src/sage/groups/libgap_mixin.py Co-authored-by: Ruchit Jagodara <ruchit.jagodara@iitgn.ac.in> * Update src/sage/groups/libgap_mixin.py Co-authored-by: Ruchit Jagodara <ruchit.jagodara@iitgn.ac.in> * Update src/sage/groups/libgap_mixin.py Co-authored-by: Ruchit Jagodara <ruchit.jagodara@iitgn.ac.in> * Update libgap_mixin.py * Update libgap_mixin.py * Update src/sage/categories/groups.py Co-authored-by: Ruchit Jagodara <ruchit.jagodara@iitgn.ac.in> * Update libgap_mixin.py * Update libgap_mixin.py --------- Co-authored-by: Ruchit Jagodara <ruchit.jagodara@iitgn.ac.in>
@fingolfin, can you please review the PR now? I have updated the algorithm, which now uses the chief series approach. The changes have made the algorithm very fast now, so thank you for your advice, and I also apologise for the delay. Also, I am very thankful to @pranav-joshi-iitgn, who helped me with this PR. |
As, @tscrim told we have used latex format for documenting the algorithm but it is now showing some kind of linting error and also failing the build documentation test, so can you please help us with this? |
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.
There can be a bit less of LaTeX in the ALGORITHM section. Something like this maybe ? :
ALGORITHM:
First we cover the cases when we can use the ``MinimalGeneratingSet``
method of GAP directly. This happens when `G` is either Simple,
or Solvable, or Nilpotent. Then, if ``libgap.MinimalGeneratingSet``
fails, we proceed assuming that `G` is not a simple group.
So, we are guaranteed to find a chief series of length at least 2.
`S := ChiefSeries(G) = [G,G_1,G_2 ... G_l]` where `G_l = \{ e \}`
Let `g` be the set of representatives of the minimum generating set
of `G/G_1`. This can be found easily (since `G/G_1` is simple group)
as ``g = libgap.MinimalGeneratingSet(GbyG1)``
for k = 2 to `l` , compute `G/G_k` , `G_{k-1}/G_k` and set
`g := lift( g, (G_{k-1}/G_k) , (G/G_k) )`
return `g`
`lift` function details:
It computes the minimum generating set (as representative elements) of
a quotient group `G/G_i` in a chief series, given the
minimum generating set (as representatives) of `G/G_{i-1}`, namely `g`
(what we are calling ``G_by_Gim1_mingen_reps`` in the code).
the factor group `G_{i-1}/G_i` and the quotient group `G/G_i` itself.
The function does these steps:
First, we compute some essential quantities:
`n :=\{ n_1,n_2 ... n_k \}`
where `\{ n_1 G_i,n_2 G_i ... n_k G_i \}` is any generating set of
`G_{i-1}/G_i` , i.e. it's the representative elements of any prefferably
small, but not necessarily minimal generating set of `G_{i-1}/G_i`
`N := \{ N_1,N_2 ... N_m \}` where
`G_{i-1}/G_i = \{N_1 G_i,N_2 G_2 ... N_m G_m \}`.
This is simply a list of representative elements of `G_{i-1}/G_i`.
We wish to find the representatives of a minimum generating set of `G/G_i`.
Here, we have two cases to consider.
First, if `G_{i-1}/G_i` is abelian :
if `<gG_i> = G/G_i`, return `g`
for `0 < p < s+1` and `n_j` in `n`, we calculate
`g^* := \{ g_1,g_2 ... g_{p-1} ,g_p n_j,g_{p_1}, ... \}`.
If `<g^* G_i> = G/G_i` , return `g^*`
Second, if `G_{i-1}` is not abelian:
First, for all combinations of (not necessarily distinct) elements
`N_{i_1},N_{i_2}... N_{i_t}` in `N`, compute
`g^* = \{g_1N_{i_1},g_{i_2}N_{i_3}... g_{i_t}N_t,g_{t+1}... g_s\}`
(This is done using the ``gen_combinations`` generator).
If `\{x G_i | x \in g^* \}` generates `G/G_i`, return `g^*`
Then, for all combinations of (not necessarily distinct) elements
`N_{i_1},N_{i_2}... N_{i_t} N_{i_{t+1}}` in `N`, compute
`g^* = \{g_1N_{i_1},g_{i_2}N_{i_3}... g_{i_t}N_t,g_{t+1}... g_s\}`
(This is done using the ``gen_combinations`` generator).
If `\{x G_i | x \in g^* \}` generates `G/G_i`, return `g^*`
By now, we must have exhausted our search.
src/sage/groups/libgap_mixin.py
Outdated
@@ -968,175 +968,171 @@ def is_isomorphic(self, H): | |||
""" | |||
return self.gap().IsomorphismGroups(H.gap()) != libgap.fail | |||
|
|||
def minimum_generating_set(G: GapElement) -> list: | |||
r""" |
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.
Probably not a good idea to make it a normal string, rather than a raw string considering all the LaTeX in the docstring. It leads to linting errors like these :
./sage/groups/libgap_mixin.py:983:39: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:983:64: W605 invalid escape sequence '\{'
./sage/groups/libgap_mixin.py:983:67: W605 invalid escape sequence '\}'
./sage/groups/libgap_mixin.py:986:64: W291 trailing whitespace
./sage/groups/libgap_mixin.py:1002:12: W605 invalid escape sequence '\{'
./sage/groups/libgap_mixin.py:1002:21: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1002:30: W605 invalid escape sequence '\}'
./sage/groups/libgap_mixin.py:1002:41: W605 invalid escape sequence '\{'
./sage/groups/libgap_mixin.py:1002:58: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1002:72: W605 invalid escape sequence '\}'
./sage/groups/libgap_mixin.py:1006:13: W605 invalid escape sequence '\{'
./sage/groups/libgap_mixin.py:1006:22: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1006:31: W605 invalid escape sequence '\}'
./sage/groups/libgap_mixin.py:1006:56: W605 invalid escape sequence '\{'
./sage/groups/libgap_mixin.py:1006:71: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1006:84: W605 invalid escape sequence '\}'
./sage/groups/libgap_mixin.py:1016:12: W605 invalid escape sequence '\l'
./sage/groups/libgap_mixin.py:1016:18: W605 invalid escape sequence '\l'
./sage/groups/libgap_mixin.py:1016:35: W605 invalid escape sequence '\i'
./sage/groups/libgap_mixin.py:1016:57: W291 trailing whitespace
./sage/groups/libgap_mixin.py:1017:13: W605 invalid escape sequence '\{'
./sage/groups/libgap_mixin.py:1017:23: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1017:54: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1017:59: W605 invalid escape sequence '\}'
./sage/groups/libgap_mixin.py:1022:21: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1022:35: W605 invalid escape sequence '\i'
./sage/groups/libgap_mixin.py:1023:12: W605 invalid escape sequence '\{'
./sage/groups/libgap_mixin.py:1023:39: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1023:63: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1023:72: W605 invalid escape sequence '\}'
./sage/groups/libgap_mixin.py:1027:21: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1027:47: W605 invalid escape sequence '\i'
./sage/groups/libgap_mixin.py:1028:12: W605 invalid escape sequence '\{'
./sage/groups/libgap_mixin.py:1028:39: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1028:63: W605 invalid escape sequence '\d'
./sage/groups/libgap_mixin.py:1028:72: W605 invalid escape sequence '\}'
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.
I have converted the docstring from normal string to raw string; thank you for your suggestion!
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.
But still doctests are failing, any idea why ?
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.
Note that I am not a member of the SageMath team, and normally don't do reviews here (nor do my reviews here carry any weight), but since you asked, here is some feedback. Overall I found the explanation of the algorithm difficult to follow (and gave up trying to do so) as there were too many phrases I found confusing and unclear. Once you clarified you abundant use of "representative objects" and fixed the "a" vs "the" issues it hopefully will be better.
@@ -451,6 +451,54 @@ def conjugacy_class(self, g): | |||
from sage.groups.conjugacy_classes import ConjugacyClass | |||
return ConjugacyClass(self, g) | |||
|
|||
def minimum_generating_set(self): | |||
""" | |||
Return a list of the minimum generating set of this group. |
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.
What does "a list of a set" mean? Perhaps instead like this?
Return a list of the minimum generating set of this group. | |
Return a minimum generating set of this group. |
If you want to emphasize that it returns a list object and not a set object, I'd be explicit about that, e.g. "a min. gen. set, represent by a list of group elements" (though IMHO that's unnecessary)
@@ -266,6 +266,7 @@ cdef class Group(Parent): | |||
""" | |||
raise NotImplementedError | |||
|
|||
|
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.
src/sage/groups/libgap_mixin.py
Outdated
the minimum generating set problem" by Bireswar Das and Dhara Thakkar (:doi:`10.48550/arXiv.2305.08405`). | ||
|
||
When group ``G`` is a simple, solvable or nilpotent then we directly use | ||
the MinimalGeneratingSet funtion from GAP which gives us the minimum generating set |
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.
the MinimalGeneratingSet funtion from GAP which gives us the minimum generating set | |
the MinimalGeneratingSet function from GAP which gives us a minimum generating set |
of that group. | ||
|
||
If the MinimalGeneratingSet function of GAP gives any error, then it is guaranteed | ||
that the group ``G`` is not a simple group and it will have a cheif series of length 2. |
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.
that the group ``G`` is not a simple group and it will have a cheif series of length 2. | |
that the group ``G`` is not a simple group and it will have a chief series of length at least 2. |
src/sage/groups/libgap_mixin.py
Outdated
|
||
`S := ChiefSeries(G) = [G,G_1,G_2 \dots G_l]` where `G_l = \{e\}` | ||
|
||
Let `g` be the set of representatives of the minimum generating set of `G/G_1`. |
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.
"the" versus "a" everywhere, I won't mark it all, please go through the text yourself looking for that
src/sage/groups/libgap_mixin.py
Outdated
We follow the algorithm described in the research paper "Algorithms for | ||
the minimum generating set problem" by Bireswar Das and Dhara Thakkar (:doi:`10.48550/arXiv.2305.08405`). | ||
|
||
When group ``G`` is a simple, solvable or nilpotent then we directly use |
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.
Note that nilpotent implies solvable so listing it in this sentence is redundant.
When group ``G`` is a simple, solvable or nilpotent then we directly use | |
When the group ``G`` is simple or solvable then we directly use |
src/sage/groups/libgap_mixin.py
Outdated
First, we compute some essential quantities: | ||
|
||
`{n} :=\{n_1,n_2\dots n_k\}` where `\{n_1 G_i,n_2G_i \dots n_kG_{i}\}` is any generating set of | ||
`G_{i-1}/G_i , i.e. it's the representative elements of any prefferably small, but not |
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.
`G_{i-1}/G_i , i.e. it's the representative elements of any prefferably small, but not | |
`G_{i-1}/G_i , i.e. it is the representative elements of any preferably small, but not |
Just a typo fix and a style suggestion, but even with that this sentence reads weird to me, and I don't like the expression "representative elements" (I don't know what really is meant by it)
src/sage/groups/libgap_mixin.py
Outdated
necessarily minimal generating set of `G_{i-1}/G_i` | ||
|
||
`{N} := \{N_1,N_2\dots N_m\}` where `G_{i-1}/G_i = \{N_1G_i,N_2G_2\dots N_m G_m\}`. | ||
This is simply a list of representative elements of `G_{i-1}/G_i`. |
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.
What are "representative elements" of a quotient G/N
??
sage: s = minimum_generating_set(G); s | ||
[(2,3), (1,3,2)(4,5)] | ||
sage: s[0].parent() | ||
C library interface to GAP |
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.
Why does it say that here?
Thank you for your feedback, I will try to make it better. |
Thank you for taking the time to look at this all the same. I am not an expert in computational group theory, so it is good to have someone who understands what parts to look at and where complexity can arise. Additionally, since we have a long standing policy of "anyone can review", your review carries the same weight as one from me. If I had to guess, "representative objects" means coset representatives. @RuchitJagodara You should compile the doc (unfortunately our CI is currently broken, so it cannot be done through here like before and must be done locally) and look at that. There are lots of missing commas and ill-formatted portions from what I can see. Also, I strong advise people to avoid |
* Update libgap_mixin.py * Update libgap_mixin.py * Update libgap_mixin.py * Update libgap_mixin.py * Update libgap_mixin.py * Update libgap_mixin.py
* Update groups.py * Update libgap_mixin.py
@tscrim, can you please help me with the failing build and test check, I am not able to understand what is the exact problem here. |
The CI testing is currently broken. |
Oh, right! Can you please review the PR and tell me if it needs any further modifications? |
For anyone interested, I have implemented the same algorithm in GAP, which I felt is a better place to put it. |
@pranav-joshi-iitgn have you opened a pr or contributed your gap code into gap, if yes then I think it would be better to close this pr and if not then I request @tscrim to review this pr. |
The code is still in the process of being reviewed. Besides, the version of GAP that sage uses needs to be updated for the method |
This patch is about implementing a minimum generating set function for group theory in Sage. The function implemented here operates in polynomial time, which makes it more interesting. Previously, we only knew of the trivial method, which was exponential in time. However, based on research paper, we can implement a
polynomial time algorithm for the minimum generating set function, and that's what this PR is about.
fixes #37362
📝 Checklist
⌛ Dependencies
NA