sage.combinat.permutation.from_cycles produces wrong result when 'cycles' is a generator

[maxale]

We have

sage: cycles = [[1,2,3],[4,5,6]]
sage: sage.combinat.permutation.from_cycles(6,cycles)
[2, 3, 1, 5, 6, 4]
sage: sage.combinat.permutation.from_cycles(6,(c for c in cycles))
[1, 2, 3, 4, 5, 6]

where the former result is correct and the latter is wrong.

Component: combinatorics

Author: Dave Morris

Branch/Commit: af2f762

Reviewer: Travis Scrimshaw

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

[DaveWitteMorris]

Branch: public/34662

[DaveWitteMorris]

Author: Dave Morris

[DaveWitteMorris]

comment:3

Thanks for pointing out this bug. I rewrote the code to only access the input once, so it works with generators. The new code also verifies the input without sorting a list (which takes more than linear time).

---

New commits:

e5e8cd5

trac 34662: create permutation from a generator

[DaveWitteMorris]

Commit: e5e8cd5

[DaveWitteMorris]

Changed stopgaps from todo to none

[tscrim]

comment:4

The test of int(x) doesn't quite do what it is claiming to do:

sage: int(1.5)
1

It would be better to just explicit convert the inputs to ZZ, which can just fail.

Likewise, do p[i] = ZZ(i+1).

Please factor out the len(cycle) calls as a variable in the loop as an optimization.

[sagetrac-git]

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

af2f762

improvements suggested by reviewer

[sagetrac-git]

Changed commit from e5e8cd5 to af2f762

[DaveWitteMorris]

comment:6

Thanks for your comments. I think I made the changes that you suggested.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:7

Thanks. LGTM.

[DaveWitteMorris]

comment:8

Thanks!

[vbraun]

Changed branch from public/34662 to af2f762