Opened 5 years ago

Closed 5 years ago

#23778 closed defect (fixed)

PermutationGroup.cardinality is sometimes an int

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.1
Component: group theory Keywords:
Cc: Merged in:
Authors: Vincent Delecroix Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: e9795a0 (Commits, GitHub, GitLab) Commit: e9795a023b4d153d34f829b30440e1120f57cab7
Dependencies: Stopgaps:

Status badges

Description 

sage: P = PermutationGroup(['(1,2)','(1,3)'])
sage: P.cardinality()
6
sage: type(P.cardinality())
<type 'int'>

This prevents using the .is_one() method or any other Integer specific method.

The problem comes from the shortcuts implemented in the _order method.

Change History (12)

comment:1 Changed 5 years ago by vdelecroix

  • Branch set to u/vdelecroix/23778
  • Commit set to f441434ee4716c05a3349b47060b061796787801
  • Status changed from new to needs_review

New commits:

​f44143423778: fix cardinality for PermutationGroup

comment:2 Changed 5 years ago by tscrim

Did you also check to see how 

        cycle_tuples = []
        for g in gens:
            temp = g.cycle_tuples()
            if len(temp) != 2:
                return None
            cycle_tuples.append(temp)

compares timing-wise? At least it seems like it would be faster...

comment:3 Changed 5 years ago by vdelecroix

I don't believe that cycle_tuples would be the culprit, but your version is cleaner.

comment:4 Changed 5 years ago by git

  • Commit changed from f441434ee4716c05a3349b47060b061796787801 to 1907f8bce2fadb15a9c2487d2a09daffb3f2ca81

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

​1907f8b23778: simpler loop

comment:5 Changed 5 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

One failing test according to the patchbot: 

sage -t --long src/sage/sets/set_from_iterator.py  # 1 doctest failed

comment:6 Changed 5 years ago by tscrim

  • Status changed from needs_review to needs_work

comment:7 Changed 5 years ago by git

  • Commit changed from 1907f8bce2fadb15a9c2487d2a09daffb3f2ca81 to e9795a023b4d153d34f829b30440e1120f57cab7

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

​e9795a023778: fix a doctest in set_from_iterator.py

comment:8 Changed 5 years ago by vdelecroix

Indeed... the source code had changed.

comment:9 Changed 5 years ago by vdelecroix

Fixed!

comment:10 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:11 Changed 5 years ago by tscrim

  • Status changed from needs_review to positive_review

Thanks.

comment:12 Changed 5 years ago by vbraun

  • Branch changed from u/vdelecroix/23778 to e9795a023b4d153d34f829b30440e1120f57cab7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.