#22765 closed defect (fixed)
Fix cycle_index for permutation groups on an arbitrary domain + cleanup
Reported by:  nthiery  Owned by:  

Priority:  major  Milestone:  sage8.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, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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/sagegit2/local/lib/python2.7/sitepackages/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 5 years ago by
 Branch set to u/nthiery/fix_cycle_index_for_permutation_groups_on_non_trivial_domains
comment:2 Changed 5 years ago by
 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 5 years ago by
 Branch set to u/nthiery/fix_cycle_index_for_permutation_groups_on_non_trivial_domains
comment:4 Changed 5 years ago by
 Commit set to c1cdcb1c8ee9aab4b7c8003933e1fe93a09fe34f
 Description modified (diff)
comment:5 followup: ↓ 7 Changed 5 years ago by
wrong syntax here:
+ (see trac:`22765`)::
should be :trac:`22765`
comment:6 Changed 5 years ago by
 Commit changed from c1cdcb1c8ee9aab4b7c8003933e1fe93a09fe34f to 6d66f65d2e120367439ea6d71ff9c0e55504cedf
Branch pushed to git repo; I updated commit sha1. New commits:
6d66f65  22765: fixed reST typo

comment:7 in reply to: ↑ 5 Changed 5 years ago by
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 5 years ago by
no, sorry. I was in mode "break at first failure"
comment:9 Changed 5 years ago by
Fair enough :) Thanks though!
comment:10 Changed 5 years ago by
 Cc dimpase added
 Keywords gap permutation group Pólya enumeration added
comment:11 Changed 5 years ago by
 Status changed from needs_review to needs_work
same here:
(:wikipedia`Cycle_index`)
comment:12 Changed 5 years ago by
 Commit changed from 6d66f65d2e120367439ea6d71ff9c0e55504cedf to bbeb9db51168fe2968d23f5ca36a703b04c73620
Branch pushed to git repo; I updated commit sha1. New commits:
bbeb9db  22765: fixed another reST typo

comment:14 Changed 5 years ago by
 Commit changed from bbeb9db51168fe2968d23f5ca36a703b04c73620 to 16eb5ec8e61512d3939bddbaac452b7e605d937a
Branch pushed to git repo; I updated commit sha1. New commits:
16eb5ec  22765: use sum_of_terms for conciseness and efficiency

comment:15 Changed 5 years ago by
Oops, while updating the write up of the book, I just noticed I had not use sum_of_terms. Fixed!
comment:16 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
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 5 years ago by
 Commit changed from 16eb5ec8e61512d3939bddbaac452b7e605d937a to e7add6def823f041ea6f70391b88169aa28088e5
Branch pushed to git repo; I updated commit sha1. New commits:
e7add6d  22765: improved type checking

comment:20 Changed 5 years ago by
Oops, the test was indeed logically broken. Fixed. I used the occasion to switch to a cleaner type checking. Thanks!
comment:21 Changed 5 years ago by
 Reviewers set to Fréderic Chapoton
 Status changed from needs_review to positive_review
ok, then let it be
comment:22 Changed 5 years ago by
 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 5 years ago by
 Commit e7add6def823f041ea6f70391b88169aa28088e5 deleted
 Reviewers changed from Fréderic Chapoton to Frédéric Chapoton
New commits:
#22765: fix cycle_index for permutation groups over an arbitrary domain + cleanup