Opened 14 months ago
Closed 12 months ago
#32314 closed defect (fixed)
passing a generator to gcd() fails
Reported by:  Lorenz Panny  Owned by:  

Priority:  minor  Milestone:  sage9.5 
Component:  basic arithmetic  Keywords:  gcd, generator, list 
Cc:  Merged in:  
Authors:  Lorenz Panny  Reviewers:  Kwankyu Lee 
Report Upstream:  N/A  Work issues:  
Branch:  e6a3c5d (Commits, GitHub, GitLab)  Commit:  e6a3c5dfeca8e37ce61efe25dbb3473475ba912c 
Dependencies:  Stopgaps: 
Description (last modified by )
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.
Change History (12)
comment:1 Changed 14 months ago by
Status:  new → needs_review 

comment:2 Changed 14 months ago by
comment:3 followup: 4 Changed 14 months ago by
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 Integer
s).
comment:4 Changed 14 months ago by
Replying to lorenz:
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 ofInteger
s).
I prefer it because of the performance degradation.
comment:5 Changed 14 months ago by
Commit:  231862dc186b620cf0f00f7397cf0141b455d682 → e6a3c5dfeca8e37ce61efe25dbb3473475ba912c 

Branch pushed to git repo; I updated commit sha1. New commits:
e6a3c5d  undo earlier list cast in GCD_list

comment:7 Changed 14 months ago by
Description:  modified (diff) 

comment:8 Changed 14 months ago by
Reviewers:  → Kwankyu Lee 

Status:  needs_review → positive_review 
LGTM
comment:10 Changed 14 months ago by
Replying to chapoton:
what about the necessary new doctest ?
Feel free to set it back to needs work if you think a doctest is required.
comment:11 Changed 14 months ago by
Milestone:  sage9.4 → sage9.5 

comment:12 Changed 12 months ago by
Branch:  public/fix_gcd_on_generators → e6a3c5dfeca8e37ce61efe25dbb3473475ba912c 

Resolution:  → fixed 
Status:  positive_review → closed 
It seems this
is enough to fix the bug, and enforcing the type in
GCD_list()
is not necessary. Is it?