Opened 16 months ago
Closed 9 months ago
#25894 closed enhancement (fixed)
Add subgroup method to MatrixGroup_base
Reported by:  soehms  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  group theory  Keywords:  subgroup, ambient, matrix groups 
Cc:  tscrim, rbeezer  Merged in:  
Authors:  Sebastian Oehms  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  c1280b9 (Commits)  Commit:  c1280b9fbd954c5f43be122cafd4606872ed353c 
Dependencies:  Stopgaps: 
Description
Only for those matrix groups which are inherited from ParentLibGAP a subgroup
method is available. But for other matrix groups such as classical groups over rings other than finite fields or ZZ this makes sense as well.
Example:
sage: G = GL(2,QQ) sage: m = matrix(QQ, 2,2, [[3, 0],[~5,1]]) sage: S = G.subgroup([m]) Traceback (most recent call last): ... AttributeError: 'LinearMatrixGroup_generic_with_category' object has no attribute 'subgroup'
Change History (17)
comment:1 Changed 16 months ago by
 Branch set to u/soehms/matrix_groups_subgroups25894
comment:2 Changed 16 months ago by
 Commit set to 0133335af28cebd62cf34d93f8efb7872ced2fe2
 Status changed from new to needs_review
comment:3 Changed 16 months ago by
 Commit changed from 0133335af28cebd62cf34d93f8efb7872ced2fe2 to 747bc167ecc5986f9426e8763fc89b0ef75beba3
Branch pushed to git repo; I updated commit sha1. New commits:
747bc16  removed unused keyword

comment:4 followup: ↓ 5 Changed 9 months ago by
Sorry for letting this ticket drop off my radar. Some comments:
 It is not good practice to have a bare
except:
. Could you give what error(s) you expect it to raise?  Error messages should start with lower case letters (following a Python convention).
SubGroup
>subgroup
for PEP8.if ambient_group == None:
>if ambient_group is None:
. Is the
if hasattr(self, '_ambient'):
test really necessary?  In general, is that entire block even necessary? I feel like there shouldn't be a need to not extend this output. Or is there a reason I am not seeing?
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 9 months ago by
Replying to tscrim:
Sorry for letting this ticket drop off my radar.
No problem (I know you have a long list)!
 It is not good practice to have a bare
except:
. Could you give what error(s) you expect it to raise?if ambient_group == None:
>if ambient_group is None:
.
I've learned that in the meantime! Sorry, for not revising this ticket, any more!
 Error messages should start with lower case letters (following a Python convention).
SubGroup
>subgroup
for PEP8.
Sorry! I should read the PEP8 more often, since much of it is contrary to what I am used to. You will find such things in #27302, as well. Please wait with that ticket until I have fixed them. But there are conventions in PEP8, the purpose of which I really don't understand. For example that about blanklines. IMO that reduces readability of the code. If you kwow a reason, please let me know. That would make it more easier to follow this convention.
 Is the
if hasattr(self, '_ambient'):
test really necessary? In general, is that entire block even necessary? I feel like there shouldn't be a need to not extend this output. Or is there a reason I am not seeing?
I inserted the block, since I was not sure if I should change the previous behaviour that much. Removing that block will change the representation string of subgroups of finite groups, as well. Of course, it will be improved:
Using optional=dochtml,memlimit,mpir,python2,sage Doctesting 1 file. sage t src/sage/groups/matrix_gps/matrix_group.py ********************************************************************** File "src/sage/groups/matrix_gps/matrix_group.py", line 720, in sage.groups.matrix_gps.matrix_group.MatrixGroup_gap._subgroup_constructor Failed example: G = SL2Z.subgroup([T^2]); G # indirect doctest Expected: Matrix group over Integer Ring with 1 generators ( [1 2] [0 1] ) Got: Subgroup of Special Linear Group of degree 2 over Integer Ring with 1 generators ( [1 2] [0 1] ) sage t src/sage/groups/libgap_wrapper.pyx # 1 doctest failed sage t src/sage/groups/libgap_mixin.py # 8 doctests failed sage t src/sage/groups/libgap_morphism.py # 4 doctests failed sage t src/sage/groups/matrix_gps/matrix_group.py # 1 doctest failed sage t src/sage/groups/matrix_gps/finitely_generated.py # 1 doctest failed sage t src/sage/misc/sagedoc.py # 4 doctests failed sage t src/doc/en/constructions/groups.rst # 1 doctest failed sage t src/doc/common/conf.py # 1 doctest failed
If you agree I will remove the block and fix all that doctests. But, will this be o.K. for external code (packages,..), as well?
comment:6 in reply to: ↑ 5 Changed 9 months ago by
Replying to soehms:
Replying to tscrim:
 It is not good practice to have a bare
except:
. Could you give what error(s) you expect it to raise?if ambient_group == None:
>if ambient_group is None:
.I've learned that in the meantime! Sorry, for not revising this ticket, any more!
No problem. I think it is fine to wait for a review before making such changes anyways.
 Error messages should start with lower case letters (following a Python convention).
SubGroup
>subgroup
for PEP8.Sorry! I should read the PEP8 more often, since much of it is contrary to what I am used to. You will find such things in #27302, as well. Please wait with that ticket until I have fixed them. But there are conventions in PEP8, the purpose of which I really don't understand. For example that about blanklines. IMO that reduces readability of the code. If you kwow a reason, please let me know. That would make it more easier to follow this convention.
They are guidelines, so no need to follow them exactly. Use your judgement to what you think makes the code the most readable (including consistency with other code in Sage).
 Is the
if hasattr(self, '_ambient'):
test really necessary? In general, is that entire block even necessary? I feel like there shouldn't be a need to not extend this output. Or is there a reason I am not seeing?
I inserted the block, since I was not sure if I should change the previous behaviour that much. Removing that block will change the representation string of subgroups of finite groups, as well. Of course, it will be improved:
sage t src/sage/groups/libgap_wrapper.pyx # 1 doctest failed sage t src/sage/groups/libgap_mixin.py # 8 doctests failed sage t src/sage/groups/libgap_morphism.py # 4 doctests failed sage t src/sage/groups/matrix_gps/matrix_group.py # 1 doctest failed sage t src/sage/groups/matrix_gps/finitely_generated.py # 1 doctest failed sage t src/sage/misc/sagedoc.py # 4 doctests failed sage t src/doc/en/constructions/groups.rst # 1 doctest failed sage t src/doc/common/conf.py # 1 doctest failedIf you agree I will remove the block and fix all that doctests. But, will this be o.K. for external code (packages,..), as well?
I think it is good for those tests to change as it gives more information about the groups in question and are otherwise trivial changes. So +1 to the change in behavior.
comment:7 Changed 9 months ago by
 Commit changed from 747bc167ecc5986f9426e8763fc89b0ef75beba3 to 80aabe61590f84c3aa210ecd4403a8d3a44bd288
comment:8 Changed 9 months ago by
 Commit changed from 80aabe61590f84c3aa210ecd4403a8d3a44bd288 to 1997ad77f29188e07c8e804da84d2a6747cfa5df
Branch pushed to git repo; I updated commit sha1. New commits:
1997ad7  25894: missing longdoctest fixed, as well

comment:9 followup: ↓ 11 Changed 9 months ago by
I had a thought/question. I think we should consider putting the generators of the subgroup first then the ambient group after it in the repr. Maybe something like
Subgroup with <n> generators ( <generators> ) of <ambient_group>
That way the most pertinent information is first and it is a little easier to parse. What do you think?
Also, some little details more:
TESTS: sage: TestSuite(G).run() sage: TestSuite(S).run()
should be TESTS::
What type of error are you expecting here:
try: return ParentLibGAP.subgroup(self, generators) except: pass
It is better to be explicit about what errors you want to handle.
raise ValueError("Generator %s is not in the group"%(g))
to lowercase generator
(this is a Python convention that we try to follow).
comment:10 Changed 9 months ago by
 Commit changed from 1997ad77f29188e07c8e804da84d2a6747cfa5df to 49a04afe2382e25904ab938d5baf489f42893426
Branch pushed to git repo; I updated commit sha1. New commits:
49a04af  25894: restore lost changes

comment:11 in reply to: ↑ 9 ; followup: ↓ 12 Changed 9 months ago by
Replying to tscrim:
I had a thought/question. I think we should consider putting the generators of the subgroup first then the ambient group after it in the repr. Maybe something like
Subgroup with <n> generators ( <generators> ) of <ambient_group>That way the most pertinent information is first and it is a little easier to parse. What do you think?
I agree! The reason why I choose it that way was to be consistent with PermutationGroup_subgroup
. I think we should have it accordingly. So your suggestion would mean to change that representation string, as well. Shall I do that?
What type of error are you expecting here:
try: return ParentLibGAP.subgroup(self, generators) except: passIt is better to be explicit about what errors you want to handle.
raise ValueError("Generator %s is not in the group"%(g))
to lowercasegenerator
(this is a Python convention that we try to follow).
Sorry again, I was sure to have those things fixed, but unfortunately I've lost them since my nice mint coloured Acer One Happy
was damaged last week (the display crashed because of a full brake application in the subway). I think I lost these changes when migrating to the new computer.
comment:12 in reply to: ↑ 11 Changed 9 months ago by
Replying to soehms:
Replying to tscrim:
I had a thought/question. I think we should consider putting the generators of the subgroup first then the ambient group after it in the repr. Maybe something like
Subgroup with <n> generators ( <generators> ) of <ambient_group>That way the most pertinent information is first and it is a little easier to parse. What do you think?
I agree! The reason why I choose it that way was to be consistent with
PermutationGroup_subgroup
. I think we should have it accordingly. So your suggestion would mean to change that representation string, as well. Shall I do that?
Yes, and yes please. (I should note that it is somewhat awkward English, so that might be why PermutationGroup_subgroup
is that way. Although I think that should also (eventually) change for the same reasons.)
What type of error are you expecting here:
try: return ParentLibGAP.subgroup(self, generators) except: passIt is better to be explicit about what errors you want to handle.
raise ValueError("Generator %s is not in the group"%(g))
to lowercasegenerator
(this is a Python convention that we try to follow).Sorry again, I was sure to have those things fixed, but unfortunately I've lost them since my nice mint coloured
Acer One Happy
was damaged last week (the display crashed because of a full brake application in the subway). I think I lost these changes when migrating to the new computer.
No problem. I am sorry to hear about your computer, but I am glad you are okay.
comment:13 Changed 9 months ago by
 Commit changed from 49a04afe2382e25904ab938d5baf489f42893426 to 455bd170e362b4e26cbc2804a083dd2a5e97766b
comment:14 Changed 9 months ago by
 Cc rbeezer added
 Milestone changed from sage8.4 to sage8.7
 Reviewers set to Travis Scrimshaw
One last little thing: could you change the doctests in the tests/books/judsonabstractalgebra/*
to be close to how they were previously? This way we fit (close to) the 80 char/line. For example
Subgroup generated by [(2,18)(3,17)(4,16)(5,15)(6,14)(7,13)(8,12)(9,11), (1,10)(2,11)(3,12)(4,13)(5,14)(6,15)(7,16)(8,17)(9,18)] of (Dihedral group of order 36 as a permutation group)
Once all of those are changed, you can set this to a positive review.
Rob, I am ccing you since you were the one who originally added these book tests.
comment:15 Changed 9 months ago by
 Commit changed from 455bd170e362b4e26cbc2804a083dd2a5e97766b to c1280b9fbd954c5f43be122cafd4606872ed353c
Branch pushed to git repo; I updated commit sha1. New commits:
c1280b9  25894: corrections on doctest layout

comment:16 Changed 9 months ago by
 Status changed from needs_review to positive_review
Thanks for the review, Travis!
comment:17 Changed 9 months ago by
 Branch changed from u/soehms/matrix_groups_subgroups25894 to c1280b9fbd954c5f43be122cafd4606872ed353c
 Resolution set to fixed
 Status changed from positive_review to closed
I add the
subgroup
method to the MatrixGroup_base class, but I avoid overwriting the code from ParentLibGAP. In analogy to the permutation groups I also modify the__repr__
method such that for a subgroup the ambient group is referred. But for instances of ParentLibGAP this would change existing behavior and many docstrings. Therefore, I exclude this case, even though I would prefer a corrected representation string in that cases, as well.With respect to the
ambient
method I don't distinguish the case ParentLibGAP, since this a just a few lines of identical code. Maybe, this better should be moved to the categorial framework.New commits:
implement subgroup / ambient methods for MatrixGroup_base