Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Holomorph method for Permutation Groups #13367

Closed
sagetrac-khalasz mannequin opened this issue Aug 14, 2012 · 19 comments
Closed

Add Holomorph method for Permutation Groups #13367

sagetrac-khalasz mannequin opened this issue Aug 14, 2012 · 19 comments

Comments

@sagetrac-khalasz
Copy link
Mannequin

sagetrac-khalasz mannequin commented Aug 14, 2012

Adds a method generating a permutation group's holomorph as a permutation group.
The holomorph of a group is the semidirect product of itself with its automorphism group, where the automorphism group acts canonically. This method has been inserted into categories/groups.py with a NotImplementedError, and then has been overridden in the case of permutation groups.

Apply

  1. attachment: 13367_holomorph.patch
  2. attachment: trac_13367_review_fc.patch

CC: @rbeezer @benjaminfjones

Component: group theory

Author: Kevin Halasz

Reviewer: Frédéric Chapoton, Benjamin Jones

Merged: sage-5.8.beta1

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

@sagetrac-khalasz sagetrac-khalasz mannequin added this to the sage-5.8 milestone Aug 14, 2012
@sagetrac-khalasz

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Dec 14, 2012

comment:3

Typo: semidrirect instead of semidirect.

Another minor nitpick: Please don't add empty lines at the end of docstring or at the beginning of the function body. Python style is to use empty lines as sparingly as possible (see PEP8).

@fchapoton
Copy link
Contributor

comment:4

Typo and doctest corrected in the review patch.

There remains to add doctest for the nonImplemented Method.

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@fchapoton

This comment has been minimized.

@sagetrac-khalasz
Copy link
Mannequin Author

sagetrac-khalasz mannequin commented Feb 22, 2013

comment:6

Sorry for forgetting about this patch. It appears that doctests have been added for both nonImplemented methods. Is it now up to me to determine if this patch should receive a positive review?

@benjaminfjones
Copy link
Contributor

comment:7

I've been lurking on this ticket for a while so I figured I could put in some effort :) It looks good now, positive review.

@benjaminfjones
Copy link
Contributor

Changed reviewer from Frédéric Chapoton to Frédéric Chapoton, Benjamin Jones

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

comment:9

The reviewer patch needs a proper commit message.

@fchapoton
Copy link
Contributor

comment:10

oops, sorry. Here is the new review patch, with added commit message

@jdemeyer
Copy link

comment:11

You should replace the commit message, not add to it. I.e. remove the second line

[mq]: trac_13367_review_fc.patch

@fchapoton
Copy link
Contributor

Attachment: trac_13367_review_fc.patch.gz

@fchapoton
Copy link
Contributor

comment:12

ok, ok. Here is a new tentative. Last one hopefully.

@jdemeyer
Copy link

Merged: sage-5.8.beta1

@jdemeyer
Copy link

Attachment: 13367_holomorph.patch.gz

@jdemeyer
Copy link

comment:14

The patch which has actually been merged, rebased to sage-5.8.beta0

@fchapoton
Copy link
Contributor

comment:15

Jeroen, did you merge the first patch only ?

Maybe you mean that you have merged both this patch and the review patch ? Then it would be ok, of course..

@jdemeyer
Copy link

comment:16

Yes, both are merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants