Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#22765 closed defect (fixed)

Fix cycle_index for permutation groups on an arbitrary domain + cleanup

Reported by: nthiery Owned by:
Priority: major Milestone: sage-8.0
Component: group theory Keywords: gap, permutation group, Pólya enumeration
Cc: nborie, dimpase Merged in:
Authors: Nicolas M. Thiéry Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: e7add6d (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by nthiery)

The code for cycle_index predates permutation groups with arbitrary domain, as well as their cycle_type methods. Instead it goes through plain Permutation's, which only works if the domain is 1,...n:

sage: G = PermutationGroup([['b','c','a']], domain=['a','b','c'])
sage: G.cycle_index()
ValueError                                Traceback (most recent call last)
/opt/sage-git2/local/lib/python2.7/site-packages/sage/combinat/permutation.pyc in __init__(self, parent, l, check_input)
    519             for i in lst:
    520                 try:
--> 521                     i = int(i)
    522                 except TypeError:
    523                     raise ValueError("the elements must be integer variables")

ValueError: invalid literal for int() with base 10: 'a'

This ticket fixes that, and uses the occasion to clean a bit the code, using in particular the conjugacy_classes method to avoid a direct access to GAP, and the docstring.

This bug was discovered while updating the book "Calcul Mathématique avec Sage".

Change History (23)

comment:1 Changed 3 years ago by nthiery

  • Branch set to u/nthiery/fix_cycle_index_for_permutation_groups_on_non_trivial_domains

comment:2 Changed 3 years ago by nthiery

  • Branch u/nthiery/fix_cycle_index_for_permutation_groups_on_non_trivial_domains deleted
  • Status changed from new to needs_review
  • Summary changed from Fix cycle_index for permutation groups on non trivial domains to Fix cycle_index for permutation groups on an arbitrary domain + cleanup

comment:3 Changed 3 years ago by nthiery

  • Branch set to u/nthiery/fix_cycle_index_for_permutation_groups_on_non_trivial_domains

comment:4 Changed 3 years ago by nthiery

  • Commit set to c1cdcb1c8ee9aab4b7c8003933e1fe93a09fe34f
  • Description modified (diff)

New commits:

c1cdcb1#22765: fix cycle_index for permutation groups over an arbitrary domain + cleanup

comment:5 follow-up: Changed 3 years ago by chapoton

wrong syntax here:

+            (see trac:`22765`)::

should be :trac:`22765`

comment:6 Changed 3 years ago by git

  • Commit changed from c1cdcb1c8ee9aab4b7c8003933e1fe93a09fe34f to 6d66f65d2e120367439ea6d71ff9c0e55504cedf

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

6d66f6522765: fixed reST typo

comment:7 in reply to: ↑ 5 Changed 3 years ago by nthiery

Replying to chapoton:

wrong syntax here:

+            (see trac:`22765`)::

should be :trac:`22765`

Fixed. Merci Frédéric!

May I assume that you proofread the rest and the syntax is correct?

comment:8 Changed 3 years ago by chapoton

no, sorry. I was in mode "break at first failure"

comment:9 Changed 3 years ago by nthiery

Fair enough :-) Thanks though!

comment:10 Changed 3 years ago by nthiery

  • Cc dimpase added
  • Keywords gap permutation group Pólya enumeration added

comment:11 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

same here:

(:wikipedia`Cycle_index`)

comment:12 Changed 3 years ago by git

  • Commit changed from 6d66f65d2e120367439ea6d71ff9c0e55504cedf to bbeb9db51168fe2968d23f5ca36a703b04c73620

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

bbeb9db22765: fixed another reST typo

comment:13 Changed 3 years ago by nthiery

  • Status changed from needs_work to needs_review

Thanks :-)

comment:14 Changed 3 years ago by git

  • Commit changed from bbeb9db51168fe2968d23f5ca36a703b04c73620 to 16eb5ec8e61512d3939bddbaac452b7e605d937a

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

16eb5ec22765: use sum_of_terms for conciseness and efficiency

comment:15 Changed 3 years ago by nthiery

Oops, while updating the write up of the book, I just noticed I had not use sum_of_terms. Fixed!

comment:16 Changed 3 years ago by nthiery

For the record, the patchbot is green up to:

sage -t --long src/sage/doctest/test.py  # Timed out after testing finished
sage -t --long src/sage/calculus/calculus.py  # 1 doctest failed

which I don't see how they could not be independent.

comment:17 Changed 3 years ago by tscrim

Yes, they are independent. Frederic (or Dima), are you going to be doing a full review, if not, I can do it.

comment:18 Changed 3 years ago by chapoton

This is slightly ambiguous, and better with more parentheses:

+            elif not hasattr(parent, "term") and hasattr(parent, "sum"):

I am satisfied with the changes. I will not look further.

comment:19 Changed 3 years ago by git

  • Commit changed from 16eb5ec8e61512d3939bddbaac452b7e605d937a to e7add6def823f041ea6f70391b88169aa28088e5

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

e7add6d22765: improved type checking

comment:20 Changed 3 years ago by nthiery

Oops, the test was indeed logically broken. Fixed. I used the occasion to switch to a cleaner type checking. Thanks!

comment:21 Changed 3 years ago by chapoton

  • Reviewers set to Fréderic Chapoton
  • Status changed from needs_review to positive_review

ok, then let it be

comment:22 Changed 3 years ago by vbraun

  • Branch changed from u/nthiery/fix_cycle_index_for_permutation_groups_on_non_trivial_domains to e7add6def823f041ea6f70391b88169aa28088e5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:23 Changed 2 years ago by jdemeyer

  • Commit e7add6def823f041ea6f70391b88169aa28088e5 deleted
  • Reviewers changed from Fréderic Chapoton to Frédéric Chapoton
Note: See TracTickets for help on using tickets.