passing a generator to gcd() fails

[yyyyx4]

The following use of gcd() fails:

sage: gcd(x for x in [123,456,789])
# ...
TypeError: object of type 'generator' has no len()

At the same time, lcm() works fine with generators:

sage: lcm(x for x in [123,456,789])
4917048

The proposed fix is to make sure we consume the generator only once in gcd() and pass it to GCD_list() as a list.

Component: basic arithmetic

Keywords: gcd, generator, list

Author: Lorenz Panny

Branch/Commit: e6a3c5d

Reviewer: Kwankyu Lee

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

[kwankyu]

comment:2

It seems this

     if seq.universe() is ZZ:
-        return GCD_list(a)
+        return GCD_list(seq)
     else:
         return __GCD_sequence(seq, **kwargs)

is enough to fix the bug, and enforcing the type in GCD_list() is not necessary. Is it?

[yyyyx4]

comment:3

You're right. I moved the list cast in GCD_list to an earlier time because GCD_list was also failing on generators due to the len() call, but that could be considered intended. I can change it back if you prefer (I agree that there might be a tiny performance impact in some cases, like when you pass a massive tuple full of Integers).

[kwankyu]

comment:4

Replying to @yyyyx4:

I can change it back if you prefer (I agree that there might be a tiny performance impact in some cases, like when you pass a massive tuple full of Integers).

I prefer it because of the performance degradation.

[sagetrac-git]

Changed commit from 231862d to e6a3c5d

[sagetrac-git]

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

e6a3c5d

undo earlier list cast in GCD_list

[yyyyx4]

comment:6

Done.

[kwankyu]

comment:8

LGTM

[kwankyu]

Reviewer: Kwankyu Lee

[fchapoton]

comment:9

what about the necessary new doctest ?

[kwankyu]

comment:10

Replying to @fchapoton:

what about the necessary new doctest ?

Feel free to set it back to needs work if you think a doctest is required.

[vbraun]

Changed branch from public/fix_gcd_on_generators to e6a3c5d