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

Profile of a Finite Permutation Group #28172

Closed
falque mannequin opened this issue Jul 11, 2019 · 31 comments
Closed

Profile of a Finite Permutation Group #28172

falque mannequin opened this issue Jul 11, 2019 · 31 comments

Comments

@falque
Copy link
Mannequin

falque mannequin commented Jul 11, 2019

Add methods profile and profile_series (= profile_polynomial) in category finite_permutation_groups, in order to be able to compute the orbital profile, and its generating series/polynomial, of a finite permutation group.

(The profile of G maps any non negative integer n onto the number of G-orbits of n-subsets, for the induced action of G on all subsets of its domain.)

CC: @nthiery @nadialafreniere

Component: group theory

Keywords: permutation groups, profile, fpsac2019

Author: Justine Falque

Branch/Commit: 609ea59

Reviewer: Frédéric Chapoton

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

@falque falque mannequin added this to the sage-8.9 milestone Jul 11, 2019
@falque falque mannequin added c: group theory labels Jul 11, 2019
@falque
Copy link
Mannequin Author

falque mannequin commented Jul 11, 2019

@fchapoton
Copy link
Contributor

Commit: fde25b1

@fchapoton
Copy link
Contributor

comment:2

after EXAMPLES:: the doctests must be indented by 4 spaces

in "profile", add INPUT to explain whar is n

add doctests with another variable name

add doc and doctests for "profile_series"


New commits:

e9b8464added profile and polynomial_of_profile methods to class PermutationGroup_generic
fde25b128172 added methods profile and profile_polynomial to class PermutationGroup_generic

@fchapoton
Copy link
Contributor

comment:3

if profile_series is just an alias, you can write

profile_series = profile_polynomial

@nthiery
Copy link
Contributor

nthiery commented Jul 12, 2019

comment:4

Thanks Frédéric for the review! I also gave some oral feedback to Justine.

if profile_series is just an alias, you can write

profile_series = profile_polynomial

Yup, with the caveat that if a subclass overrides profile_polynomial, profile_series will still point to the one here.

@falque

This comment has been minimized.

@falque
Copy link
Mannequin Author

falque mannequin commented Jul 12, 2019

Changed branch from u/falque/profile_of_a_finite_permutation_group to none

@falque
Copy link
Mannequin Author

falque mannequin commented Jul 12, 2019

Changed commit from fde25b1 to none

@falque
Copy link
Mannequin Author

falque mannequin commented Jul 12, 2019

comment:5

Methods should be added to the category of finite permutation groups rather than the class PermutationGroup_generic

@falque
Copy link
Mannequin Author

falque mannequin commented Jul 12, 2019

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2019

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

6ede59aImprovements in the documentation and more flexibility about the nature of variable

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2019

Commit: 6ede59a

@nadialafreniere
Copy link
Contributor

Changed keywords from permutation groups, profile to permutation groups, profile, fpsac2019

@fchapoton
Copy link
Contributor

comment:9

needs review ?

@falque falque mannequin added the s: needs review label Jul 28, 2019
@nadialafreniere
Copy link
Contributor

comment:12

Great job Justine! I'm glad you added it to Sage.
I have some comments for you:

  • It should be clear in the doc what the profile is. Please, add it to the docstring of profile_polynomial.
  • There could be more examples in profile_polynomial. For now, the only example is with the fifth cyclic group, but it would be nice to have it with something else. For example, C6 would be interesting, since it makes it clear that we are looking only at cycles and no other symmetries, like with D6 (it was not exactly clear to me after the computation for C5, since the profile polynomial is the same as for D5). The Symmetric or Dihedral groups could be good examples.
  • It is not clear to me what the added value of the profile_series function is. Doesn't it do the same as profile_polynomial? If so, I like the idea of an alias. If you are worried about someone changing profile_polynomial, it is still an issue in the way it is written right now.
  • I don't know how to do it efficiently, but I am wondering if it really is a good idea to compute the whole polynomial just to get one coefficient in profile. If there is a faster way to do it, especially when the group is big, please change the code.
  • There are some trailing spaces at the end of lines, and they should be erased.
    Congrats againt for this contribution!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2019

Changed commit from 6ede59a to 012740b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2019

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

012740bRevised docstring, particularly in profile_series, with new examples and a description of what the profile is; profile_polynomial becomes an alias; an optional argument is added in method profile to allow the user to change the computation method.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2019

Changed commit from 012740b to fd6fbb4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2019

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

fd6fbb4wrong file, sorry; corrected

@falque
Copy link
Mannequin Author

falque mannequin commented Aug 26, 2019

comment:15
  • I made the suggested additions to the documentation of profile_series.

  • "If you are worried about someone changing profile_polynomial, it is still an issue in the way it is written right now."
    --> I was told differently indeed; anyway, I think it is clearer with
    an alias for now, so I changed it.

  • I checked the useless spaces, it should be ok.

  • "I don't know how to do it efficiently, but I am wondering if it really
    is a good idea to compute the whole polynomial just to get one coefficient
    in profile. If there is a faster way to do it, especially when the group
    is big, please change the code."
    --> There is indeed a more direct method, although I would call it the
    "naive" method since I am not sure it is really faster than Polyà
    enumeration, which presents in addition the bonus of computing all
    values of the profile in a row. I added nevertheless an optional argument
    to allow the user to use their preferred method. This can also make what
    the profile is clearer to the user !

  • some micro changes here and there

@falque

This comment has been minimized.

@falque falque mannequin added s: needs review and removed s: needs work labels Aug 26, 2019
@fchapoton
Copy link
Contributor

comment:16
  • Latex syntax is not allowed in the doc, but unicode works fine : instead of P'{o}ly
    `{a}
    , just use Pólya.

  • You could add some cross-references using
    .. SEEALSO:: :meth:`profile` and
    .. SEEALSO:: :meth:`profile_series`
    in the appropriate place of the docs.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2019

Changed commit from fd6fbb4 to 798f9c0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2019

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

798f9c0Added references, fixed one or two typos and errors

@fchapoton
Copy link
Contributor

comment:18

Pólya takes just one accent:

https://fr.wikipedia.org/wiki/George_P%C3%B3lya

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2019

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

609ea59typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2019

Changed commit from 798f9c0 to 609ea59

@fchapoton
Copy link
Contributor

comment:20

ok, looks good to me. Merci !

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@falque
Copy link
Mannequin Author

falque mannequin commented Aug 29, 2019

comment:21

Replying to @fchapoton:

ok, looks good to me. Merci !

Thank you !!!

@vbraun
Copy link
Member

vbraun commented Sep 2, 2019

Changed branch from u/falque/profile_of_a_finite_permutation_group to 609ea59

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

4 participants