Opened 8 years ago

Closed 8 years ago

#14910 closed enhancement (fixed)

as_finitely_presented_group method for permutation groups

Reported by: dshurbert Owned by:
Priority: minor Milestone: sage-5.12
Component: group theory Keywords: group presentations
Cc: rbeezer, vbraun, mmarco Merged in: sage-5.12.beta3
Authors: Davis Shurbert Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14790 Stopgaps:

Status badges

Description (last modified by dshurbert)

The permutation group method to_presentation wraps the GAP function IsomorphismFpGroupByGenerators? which yields an isomorphism from a given group to an isomorphic finite presentation. to_presentation takes the image of the isomorphism returned by this function and wraps it as a Sage finitely presented group.

sage: G = DihedralGroup(8); G
Dihedral group of order 16 as a permutation group
sage: G.to_presentation()
Finitely presented group < a, b | b^2, b*a^-1*b*a^-1, a^8 >

Depends on #14790 which gives the python generator object needed for variable names.

Apply

1 trac_14910_to_presentation.patch

Attachments (1)

trac_14910_to_presentation.patch (5.5 KB) - added by dshurbert 8 years ago.
Replacement patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by dshurbert

  • Description modified (diff)

comment:2 Changed 8 years ago by dshurbert

Variable names for free group generators are manually changed from an "f.1" and "f.2" style to an "a" and "b" style. I use sage_eval and a dictionary binding old generator names carried over from GAP to lexicographical generator names. If anyone has a better way to do this renaming process please don't hesitate to comment.

comment:3 Changed 8 years ago by dshurbert

  • Description modified (diff)

comment:4 Changed 8 years ago by vbraun

I don't like the whole conversion to string and then evaluating it. You should convert the generators to Tietze words which are just tuples of integers and hence don't depend on the generator names.

For consistency with the other group theory code, I'd rather call the method as_finitely_presented_group(). For example, we also have

sage: G.<a,b> = FreeGroup()
sage: H = G / (a^2, b^3, a*b*~a*~b)
sage: H.as_permutation_group()
Permutation Group with generators [(1,2)(3,5)(4,6), (1,3,4)(2,5,6)]

Small nitpicks:

  • Empty line after the title ("Return a finitely presented group...")
  • Two dashes after input: - ``reduced`` --`, this is the ReST code for en dash
Last edited 8 years ago by vbraun (previous) (diff)

comment:5 Changed 8 years ago by dshurbert

Just made suggested changes, using the Tietze word of each relation was definitely a better way to approach the problem. I am currently waiting on 5.11.beta3 to finish building on my machine so I can test the current output against the new printing. The same goes for #14791.

comment:6 Changed 8 years ago by rbeezer

Comments:

  1. Volker's Tietze word suggestion is a good one. Capitalize Tietze where you use it, isn't a person's name?
  1. to_ or as_? Seems Sage uses "to" more often, but you do see "as" in group theory code.
  1. Drop # in text of TESTS in doc tests and make this actual text that will appear properly in documentation.
  1. When you illustrate "reduced" in doctest, write (reduced=True) to make it clearer what you are illustrating.
  1. Drop sage_eval import. Check othrs-
  1. In code. I'm sympathetic to aligning equals sign in four assignments, but I would drop it.

comment:7 Changed 8 years ago by dshurbert

Made requested changes. I left the method name as as_ because it fits with the fpgroup.as_permutation_group style. I rebased the patch to 5.11.beta3 and fixed errors arising from the new printing of group presentations from GAP.

comment:8 Changed 8 years ago by vbraun

  • Status changed from new to needs_review

Ready for review?

Its better to break overly long lines, e.g.

        ret_rls = tuple([F(rel_word.TietzeWordAbstractWord(image_gens).sage()) 
                         for rel_word in image_fp.RelatorsOfFpGroup()])  

instead of

        ret_rls = tuple([F(rel_word.TietzeWordAbstractWord(image_gens).sage()) for rel_word in image_fp.RelatorsOfFpGroup()])  

comment:9 Changed 8 years ago by dshurbert

Just shortened overly long line. Ready for review.

comment:10 Changed 8 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review
  • Summary changed from to_presentation method for permutation groups to as_finitely_presented_group method for permutation groups

Looks good to me

comment:11 Changed 8 years ago by jdemeyer

  • Milestone set to sage-5.12

comment:12 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The patch needs a proper commit message.

comment:13 Changed 8 years ago by jdemeyer

There is a conflict with #14791: both patches add the same reference [TWGT].

Changed 8 years ago by dshurbert

Replacement patch

comment:14 Changed 8 years ago by dshurbert

  • Status changed from needs_work to needs_review

Fixed reference conflict, added proper commit message.

comment:15 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.12.beta3
  • Resolution set to fixed
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.