Add subgroup method to MatrixGroup_base

[soehms]

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'

CC: @tscrim @rbeezer

Component: group theory

Keywords: subgroup, ambient, matrix groups

Author: Sebastian Oehms

Branch/Commit: c1280b9

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/25894

[soehms]

Branch: u/soehms/matrix_groups_subgroups-25894

[soehms]

comment:2

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 doc-strings. 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:

0133335

implement subgroup / ambient methods for MatrixGroup_base

[soehms]

Commit: 0133335

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

747bc16

removed unused keyword

[sagetrac-git]

Changed commit from 0133335 to 747bc16

[tscrim]

comment:4

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?

[soehms]

comment:5

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?

[tscrim]

comment:6

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 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?

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.

[sagetrac-git]

Changed commit from 747bc16 to 80aabe6

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

e9c6171	Merge branch 'u/soehms/matrix_groups_subgroups-25894' of git://trac.sagemath.org/sage into matrix_groups_subgroups_25894
80aabe6	25894: implement reviewers suggestions

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

1997ad7

25894: missing long-doctest fixed, as well

[sagetrac-git]

Changed commit from 80aabe6 to 1997ad7

[tscrim]

comment:9

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).

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

49a04af

25894: restore lost changes

[sagetrac-git]

Changed commit from 1997ad7 to 49a04af

[soehms]

comment:11

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:
                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).

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.

[tscrim]

comment:12

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:
                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).

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.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

c7cb49f	Merge branch 'u/soehms/matrix_groups_subgroups-25894' of git://trac.sagemath.org/sage into matrix_group_subgroups_25894
0aec3cc	change representation strings according to reviewers suggestion
455bd17	fixing further doctests

[sagetrac-git]

Changed commit from 49a04af to 455bd17

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:14

One last little thing: could you change the doctests in the tests/books/judson-abstract-algebra/* 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 cc-ing you since you were the one who originally added these book tests.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

c1280b9

25894: corrections on doctest layout

[sagetrac-git]

Changed commit from 455bd17 to c1280b9

[soehms]

comment:16

Thanks for the review, Travis!

[vbraun]

Changed branch from u/soehms/matrix_groups_subgroups-25894 to c1280b9