Sage: Ticket #25894: Add subgroup method to MatrixGroup_base
https://trac.sagemath.org/ticket/25894
<p>
Only for those matrix groups which are inherited from ParentLibGAP a <code>subgroup</code> 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.
</p>
<p>
Example:
</p>
<pre class="wiki">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'
</pre>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/25894
Trac 1.1.6soehmsSun, 22 Jul 2018 07:07:16 GMTbranch set
https://trac.sagemath.org/ticket/25894#comment:1
https://trac.sagemath.org/ticket/25894#comment:1
<ul>
<li><strong>branch</strong>
set to <em>u/soehms/matrix_groups_subgroups-25894</em>
</li>
</ul>
TicketsoehmsSun, 22 Jul 2018 07:19:40 GMTstatus changed; commit set
https://trac.sagemath.org/ticket/25894#comment:2
https://trac.sagemath.org/ticket/25894#comment:2
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
set to <em>0133335af28cebd62cf34d93f8efb7872ced2fe2</em>
</li>
</ul>
<p>
I add the <code>subgroup</code> method to the MatrixGroup_base class, but I avoid overwriting the code from ParentLibGAP. In analogy to the permutation groups I also modify the <code>__repr__</code> method such that for a subgroup the ambient group is referred. But for instances of ParentLibGAP this would change existing behavior and many doc-strings. Therefore, I exclude this case, even though I would prefer a corrected representation string in that cases, as well.
</p>
<p>
With respect to the <code>ambient</code> 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.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=0133335af28cebd62cf34d93f8efb7872ced2fe2"><span class="icon"></span>0133335</a></td><td><code>implement subgroup / ambient methods for MatrixGroup_base</code>
</td></tr></table>
TicketgitSun, 22 Jul 2018 07:24:27 GMTcommit changed
https://trac.sagemath.org/ticket/25894#comment:3
https://trac.sagemath.org/ticket/25894#comment:3
<ul>
<li><strong>commit</strong>
changed from <em>0133335af28cebd62cf34d93f8efb7872ced2fe2</em> to <em>747bc167ecc5986f9426e8763fc89b0ef75beba3</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=747bc167ecc5986f9426e8763fc89b0ef75beba3"><span class="icon"></span>747bc16</a></td><td><code>removed unused keyword</code>
</td></tr></table>
TickettscrimSat, 16 Feb 2019 17:12:07 GMT
https://trac.sagemath.org/ticket/25894#comment:4
https://trac.sagemath.org/ticket/25894#comment:4
<p>
Sorry for letting this ticket drop off my radar. Some comments:
</p>
<ul><li>It is not good practice to have a bare <code>except:</code>. Could you give what error(s) you expect it to raise?
</li><li>Error messages should start with lower case letters (following a Python convention).
</li><li><code>SubGroup</code> -> <code>subgroup</code> for PEP8.
</li><li><code>if ambient_group == None:</code> -> <code>if ambient_group is None:</code>.
</li><li>Is the <code>if hasattr(self, '_ambient'):</code> test really necessary?
</li><li>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?
</li></ul>
TicketsoehmsMon, 18 Feb 2019 08:39:27 GMT
https://trac.sagemath.org/ticket/25894#comment:5
https://trac.sagemath.org/ticket/25894#comment:5
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25894#comment:4" title="Comment 4">tscrim</a>:
</p>
<blockquote class="citation">
<p>
Sorry for letting this ticket drop off my radar.
</p>
</blockquote>
<p>
No problem (I know you have a long list)!
</p>
<blockquote class="citation">
<ul><li>It is not good practice to have a bare <code>except:</code>. Could you give what error(s) you expect it to raise?
</li><li><code>if ambient_group == None:</code> -> <code>if ambient_group is None:</code>.
</li></ul></blockquote>
<p>
I've learned that in the meantime! Sorry, for not revising this ticket, any more!
</p>
<blockquote class="citation">
<ul><li>Error messages should start with lower case letters (following a Python convention).
</li><li><code>SubGroup</code> -> <code>subgroup</code> for PEP8.
</li></ul></blockquote>
<p>
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 <a class="closed ticket" href="https://trac.sagemath.org/ticket/27302" title="enhancement: Cubic Braid Groups (closed: fixed)">#27302</a>, 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.
</p>
<blockquote class="citation">
<ul><li>Is the <code>if hasattr(self, '_ambient'):</code> test really necessary?
</li><li>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?
</li></ul></blockquote>
<p>
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:
</p>
<pre class="wiki">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
</pre><p>
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?
</p>
TickettscrimWed, 20 Feb 2019 07:04:06 GMT
https://trac.sagemath.org/ticket/25894#comment:6
https://trac.sagemath.org/ticket/25894#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25894#comment:5" title="Comment 5">soehms</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25894#comment:4" title="Comment 4">tscrim</a>:
</p>
<blockquote class="citation">
<ul><li>It is not good practice to have a bare <code>except:</code>. Could you give what error(s) you expect it to raise?
</li><li><code>if ambient_group == None:</code> -> <code>if ambient_group is None:</code>.
</li></ul></blockquote>
<p>
I've learned that in the meantime! Sorry, for not revising this ticket, any more!
</p>
</blockquote>
<p>
No problem. I think it is fine to wait for a review before making such changes anyways.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Error messages should start with lower case letters (following a Python convention).
</li><li><code>SubGroup</code> -> <code>subgroup</code> for PEP8.
</li></ul></blockquote>
<p>
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 <a class="closed ticket" href="https://trac.sagemath.org/ticket/27302" title="enhancement: Cubic Braid Groups (closed: fixed)">#27302</a>, 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.
</p>
</blockquote>
<p>
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).
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>Is the <code>if hasattr(self, '_ambient'):</code> test really necessary?
</li><li>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?
</li></ul></blockquote>
<p>
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:
</p>
<pre class="wiki">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
</pre><p>
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?
</p>
</blockquote>
<p>
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.
</p>
TicketgitSun, 24 Feb 2019 10:56:31 GMTcommit changed
https://trac.sagemath.org/ticket/25894#comment:7
https://trac.sagemath.org/ticket/25894#comment:7
<ul>
<li><strong>commit</strong>
changed from <em>747bc167ecc5986f9426e8763fc89b0ef75beba3</em> to <em>80aabe61590f84c3aa210ecd4403a8d3a44bd288</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=e9c61710b02494bdfb7d0b2a9ebd899e0ca0b2ef"><span class="icon"></span>e9c6171</a></td><td><code>Merge branch 'u/soehms/matrix_groups_subgroups-25894' of git://trac.sagemath.org/sage into matrix_groups_subgroups_25894</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=80aabe61590f84c3aa210ecd4403a8d3a44bd288"><span class="icon"></span>80aabe6</a></td><td><code>25894: implement reviewers suggestions</code>
</td></tr></table>
TicketgitSun, 24 Feb 2019 23:02:28 GMTcommit changed
https://trac.sagemath.org/ticket/25894#comment:8
https://trac.sagemath.org/ticket/25894#comment:8
<ul>
<li><strong>commit</strong>
changed from <em>80aabe61590f84c3aa210ecd4403a8d3a44bd288</em> to <em>1997ad77f29188e07c8e804da84d2a6747cfa5df</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=1997ad77f29188e07c8e804da84d2a6747cfa5df"><span class="icon"></span>1997ad7</a></td><td><code>25894: missing long-doctest fixed, as well</code>
</td></tr></table>
TickettscrimMon, 25 Feb 2019 02:19:18 GMT
https://trac.sagemath.org/ticket/25894#comment:9
https://trac.sagemath.org/ticket/25894#comment:9
<p>
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
</p>
<pre class="wiki">Subgroup with <n> generators (
<generators>
) of <ambient_group>
</pre><p>
That way the most pertinent information is first and it is a little easier to parse. What do you think?
</p>
<p>
Also, some little details more:
</p>
<pre class="wiki"> TESTS:
sage: TestSuite(G).run()
sage: TestSuite(S).run()
</pre><p>
should be <code>TESTS::</code>
</p>
<p>
What type of error are you expecting here:
</p>
<pre class="wiki"> try:
return ParentLibGAP.subgroup(self, generators)
except:
pass
</pre><p>
It is better to be explicit about what errors you want to handle.
</p>
<p>
<code>raise ValueError("Generator %s is not in the group"%(g))</code> to lowercase <code>generator</code> (this is a Python convention that we try to follow).
</p>
TicketgitMon, 25 Feb 2019 19:55:56 GMTcommit changed
https://trac.sagemath.org/ticket/25894#comment:10
https://trac.sagemath.org/ticket/25894#comment:10
<ul>
<li><strong>commit</strong>
changed from <em>1997ad77f29188e07c8e804da84d2a6747cfa5df</em> to <em>49a04afe2382e25904ab938d5baf489f42893426</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=49a04afe2382e25904ab938d5baf489f42893426"><span class="icon"></span>49a04af</a></td><td><code>25894: restore lost changes</code>
</td></tr></table>
TicketsoehmsMon, 25 Feb 2019 20:01:41 GMT
https://trac.sagemath.org/ticket/25894#comment:11
https://trac.sagemath.org/ticket/25894#comment:11
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25894#comment:9" title="Comment 9">tscrim</a>:
</p>
<blockquote class="citation">
<p>
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
</p>
<pre class="wiki">Subgroup with <n> generators (
<generators>
) of <ambient_group>
</pre><p>
That way the most pertinent information is first and it is a little easier to parse. What do you think?
</p>
</blockquote>
<p>
I agree! The reason why I choose it that way was to be consistent with <code>PermutationGroup_subgroup</code>. I think we should have it accordingly. So your suggestion would mean to change that representation string, as well. Shall I do that?
</p>
<blockquote class="citation">
<p>
What type of error are you expecting here:
</p>
<pre class="wiki"> try:
return ParentLibGAP.subgroup(self, generators)
except:
pass
</pre><p>
It is better to be explicit about what errors you want to handle.
</p>
<p>
<code>raise ValueError("Generator %s is not in the group"%(g))</code> to lowercase <code>generator</code> (this is a Python convention that we try to follow).
</p>
</blockquote>
<p>
Sorry again, I was sure to have those things fixed, but unfortunately I've lost them since my nice mint coloured <code>Acer One Happy</code> 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.
</p>
TickettscrimMon, 25 Feb 2019 22:22:59 GMT
https://trac.sagemath.org/ticket/25894#comment:12
https://trac.sagemath.org/ticket/25894#comment:12
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25894#comment:11" title="Comment 11">soehms</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/25894#comment:9" title="Comment 9">tscrim</a>:
</p>
<blockquote class="citation">
<p>
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
</p>
<pre class="wiki">Subgroup with <n> generators (
<generators>
) of <ambient_group>
</pre><p>
That way the most pertinent information is first and it is a little easier to parse. What do you think?
</p>
</blockquote>
<p>
I agree! The reason why I choose it that way was to be consistent with <code>PermutationGroup_subgroup</code>. I think we should have it accordingly. So your suggestion would mean to change that representation string, as well. Shall I do that?
</p>
</blockquote>
<p>
Yes, and yes please. (I should note that it is somewhat awkward English, so that might be why <code>PermutationGroup_subgroup</code> is that way. Although I think that should also (eventually) change for the same reasons.)
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
What type of error are you expecting here:
</p>
<pre class="wiki"> try:
return ParentLibGAP.subgroup(self, generators)
except:
pass
</pre><p>
It is better to be explicit about what errors you want to handle.
</p>
<p>
<code>raise ValueError("Generator %s is not in the group"%(g))</code> to lowercase <code>generator</code> (this is a Python convention that we try to follow).
</p>
</blockquote>
<p>
Sorry again, I was sure to have those things fixed, but unfortunately I've lost them since my nice mint coloured <code>Acer One Happy</code> 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.
</p>
</blockquote>
<p>
No problem. I am sorry to hear about your computer, but I am glad you are okay.
</p>
TicketgitWed, 27 Feb 2019 23:25:30 GMTcommit changed
https://trac.sagemath.org/ticket/25894#comment:13
https://trac.sagemath.org/ticket/25894#comment:13
<ul>
<li><strong>commit</strong>
changed from <em>49a04afe2382e25904ab938d5baf489f42893426</em> to <em>455bd170e362b4e26cbc2804a083dd2a5e97766b</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=c7cb49f96cbe981ba5b245df431abfff14b21d5f"><span class="icon"></span>c7cb49f</a></td><td><code>Merge branch 'u/soehms/matrix_groups_subgroups-25894' of git://trac.sagemath.org/sage into matrix_group_subgroups_25894</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=0aec3cc92bc13009b97bab8b42bce4772edcafb4"><span class="icon"></span>0aec3cc</a></td><td><code>change representation strings according to reviewers suggestion</code>
</td></tr><tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=455bd170e362b4e26cbc2804a083dd2a5e97766b"><span class="icon"></span>455bd17</a></td><td><code>fixing further doctests</code>
</td></tr></table>
TickettscrimFri, 01 Mar 2019 03:52:59 GMTcc, milestone changed; reviewer set
https://trac.sagemath.org/ticket/25894#comment:14
https://trac.sagemath.org/ticket/25894#comment:14
<ul>
<li><strong>cc</strong>
<em>rbeezer</em> added
</li>
<li><strong>reviewer</strong>
set to <em>Travis Scrimshaw</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-8.4</em> to <em>sage-8.7</em>
</li>
</ul>
<p>
One last little thing: could you change the doctests in the <code>tests/books/judson-abstract-algebra/*</code> to be close to how they were previously? This way we fit (close to) the 80 char/line. For example
</p>
<pre class="wiki">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)
</pre><p>
Once all of those are changed, you can set this to a positive review.
</p>
<p>
Rob, I am cc-ing you since you were the one who originally added these book tests.
</p>
TicketgitSat, 02 Mar 2019 22:09:32 GMTcommit changed
https://trac.sagemath.org/ticket/25894#comment:15
https://trac.sagemath.org/ticket/25894#comment:15
<ul>
<li><strong>commit</strong>
changed from <em>455bd170e362b4e26cbc2804a083dd2a5e97766b</em> to <em>c1280b9fbd954c5f43be122cafd4606872ed353c</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=c1280b9fbd954c5f43be122cafd4606872ed353c"><span class="icon"></span>c1280b9</a></td><td><code>25894: corrections on doctest layout</code>
</td></tr></table>
TicketsoehmsSat, 02 Mar 2019 22:11:24 GMTstatus changed
https://trac.sagemath.org/ticket/25894#comment:16
https://trac.sagemath.org/ticket/25894#comment:16
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Thanks for the review, Travis!
</p>
TicketvbraunSun, 03 Mar 2019 22:38:17 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/25894#comment:17
https://trac.sagemath.org/ticket/25894#comment:17
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>branch</strong>
changed from <em>u/soehms/matrix_groups_subgroups-25894</em> to <em>c1280b9fbd954c5f43be122cafd4606872ed353c</em>
</li>
</ul>
Ticket