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

PermutationGroup.cardinality is sometimes an int #23778

Closed
videlec opened this issue Sep 2, 2017 · 15 comments
Closed

PermutationGroup.cardinality is sometimes an int #23778

videlec opened this issue Sep 2, 2017 · 15 comments

Comments

@videlec
Copy link
Contributor

videlec commented Sep 2, 2017

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.

Component: group theory

Author: Vincent Delecroix

Branch/Commit: e9795a0

Reviewer: Travis Scrimshaw

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

@videlec videlec added this to the sage-8.1 milestone Sep 2, 2017
@videlec
Copy link
Contributor Author

videlec commented Sep 2, 2017

Commit: f441434

@videlec
Copy link
Contributor Author

videlec commented Sep 2, 2017

New commits:

f44143423778: fix cardinality for PermutationGroup

@videlec
Copy link
Contributor Author

videlec commented Sep 2, 2017

Branch: u/vdelecroix/23778

@tscrim
Copy link
Collaborator

tscrim commented Sep 2, 2017

comment:2

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...

@videlec
Copy link
Contributor Author

videlec commented Sep 3, 2017

comment:3

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2017

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

1907f8b23778: simpler loop

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2017

Changed commit from f441434 to 1907f8b

@tscrim
Copy link
Collaborator

tscrim commented Sep 4, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Sep 4, 2017

comment:5

One failing test according to the patchbot:

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2017

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

e9795a023778: fix a doctest in set_from_iterator.py

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2017

Changed commit from 1907f8b to e9795a0

@videlec
Copy link
Contributor Author

videlec commented Sep 4, 2017

comment:8

Indeed... the source code had changed.

@videlec
Copy link
Contributor Author

videlec commented Sep 4, 2017

comment:9

Fixed!

@tscrim
Copy link
Collaborator

tscrim commented Sep 4, 2017

comment:11

Thanks.

@vbraun
Copy link
Member

vbraun commented Sep 10, 2017

Changed branch from u/vdelecroix/23778 to e9795a0

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

3 participants