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: sage-8.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 soehms

  • Branch set to u/soehms/matrix_groups_subgroups-25894

comment:2 Changed 16 months ago by soehms

  • Commit set to 0133335af28cebd62cf34d93f8efb7872ced2fe2
  • Status changed from new to needs_review

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:

0133335implement subgroup / ambient methods for MatrixGroup_base

comment:3 Changed 16 months ago by git

  • Commit changed from 0133335af28cebd62cf34d93f8efb7872ced2fe2 to 747bc167ecc5986f9426e8763fc89b0ef75beba3

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

747bc16removed unused keyword

comment:4 follow-up: Changed 9 months ago by tscrim

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 ; follow-up: Changed 9 months ago by soehms

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 tscrim

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.

comment:7 Changed 9 months ago by git

  • Commit changed from 747bc167ecc5986f9426e8763fc89b0ef75beba3 to 80aabe61590f84c3aa210ecd4403a8d3a44bd288

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

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

comment:8 Changed 9 months ago by git

  • Commit changed from 80aabe61590f84c3aa210ecd4403a8d3a44bd288 to 1997ad77f29188e07c8e804da84d2a6747cfa5df

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

1997ad725894: missing long-doctest fixed, as well

comment:9 follow-up: Changed 9 months ago by 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?

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 git

  • Commit changed from 1997ad77f29188e07c8e804da84d2a6747cfa5df to 49a04afe2382e25904ab938d5baf489f42893426

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

49a04af25894: restore lost changes

comment:11 in reply to: ↑ 9 ; follow-up: Changed 9 months ago by 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?

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.

comment:12 in reply to: ↑ 11 Changed 9 months ago by tscrim

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.

comment:13 Changed 9 months ago by git

  • Commit changed from 49a04afe2382e25904ab938d5baf489f42893426 to 455bd170e362b4e26cbc2804a083dd2a5e97766b

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

c7cb49fMerge branch 'u/soehms/matrix_groups_subgroups-25894' of git://trac.sagemath.org/sage into matrix_group_subgroups_25894
0aec3ccchange representation strings according to reviewers suggestion
455bd17fixing further doctests

comment:14 Changed 9 months ago by tscrim

  • Cc rbeezer added
  • Milestone changed from sage-8.4 to sage-8.7
  • Reviewers set to Travis Scrimshaw

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.

comment:15 Changed 9 months ago by git

  • Commit changed from 455bd170e362b4e26cbc2804a083dd2a5e97766b to c1280b9fbd954c5f43be122cafd4606872ed353c

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

c1280b925894: corrections on doctest layout

comment:16 Changed 9 months ago by soehms

  • Status changed from needs_review to positive_review

Thanks for the review, Travis!

comment:17 Changed 9 months ago by vbraun

  • Branch changed from u/soehms/matrix_groups_subgroups-25894 to c1280b9fbd954c5f43be122cafd4606872ed353c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.