#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, 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/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 4 years ago by
- Branch set to u/nthiery/fix_cycle_index_for_permutation_groups_on_non_trivial_domains
comment:2 Changed 4 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 4 years ago by
- Branch set to u/nthiery/fix_cycle_index_for_permutation_groups_on_non_trivial_domains
comment:4 Changed 4 years ago by
- Commit set to c1cdcb1c8ee9aab4b7c8003933e1fe93a09fe34f
- Description modified (diff)
comment:5 follow-up: ↓ 7 Changed 4 years ago by
wrong syntax here:
+ (see trac:`22765`)::
should be :trac:`22765`
comment:6 Changed 4 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 4 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 4 years ago by
no, sorry. I was in mode "break at first failure"
comment:9 Changed 4 years ago by
Fair enough :-) Thanks though!
comment:10 Changed 4 years ago by
- Cc dimpase added
- Keywords gap permutation group Pólya enumeration added
comment:11 Changed 4 years ago by
- Status changed from needs_review to needs_work
same here:
(:wikipedia`Cycle_index`)
comment:12 Changed 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 3 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