Opened 5 years ago
Closed 5 years ago
#22630 closed defect (fixed)
Fix mutation of list in lcm
Reported by: | tscrim | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.0 |
Component: | basic arithmetic | Keywords: | days85 |
Cc: | jdemeyer | Merged in: | |
Authors: | Travis Scrimshaw | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 40e2b08 (Commits, GitHub, GitLab) | Commit: | 40e2b0835e257b55aa013d5583ef0f1121111ba1 |
Dependencies: | Stopgaps: |
Description
The current LCM_list
will mutate the list when it contains int
's (and possibly Sage integers and long
):
sage: L = [int(1), int(2)] sage: lcm(L) 2 sage: type(L[0]) <type 'sage.rings.integer.Integer'>
Change History (15)
comment:1 Changed 5 years ago by
- Branch set to public/arith/fix_lcm-22630
- Cc jdemeyer added
- Commit set to ad2189cbf2c051d3628ffeb988c231f6a7e3e45d
- Status changed from new to needs_review
comment:2 Changed 5 years ago by
- Status changed from needs_review to needs_work
First of all, do not put Python code inside sig_on()
.
comment:3 Changed 5 years ago by
If LCM_list
and LCM_sequence
handle non-integers now, they should be moved outside of src/sage/rings/integer.pyx
.
comment:4 Changed 5 years ago by
No need to deprecate __LCM_sequence
since it was private already.
comment:5 Changed 5 years ago by
- Commit changed from ad2189cbf2c051d3628ffeb988c231f6a7e3e45d to 9066ab4d06169ee7dfd0806d6208f9a8728718e3
comment:6 Changed 5 years ago by
- Status changed from needs_work to needs_review
I'm going to be paranoid and leave in the deprecation of __LCM_sequence
since someone might be using it for speed reasons (although I doubt it).
comment:7 Changed 5 years ago by
- Commit changed from 9066ab4d06169ee7dfd0806d6208f9a8728718e3 to 7603fe056c0230608dfb2a7e6b7adffbfea88b66
Branch pushed to git repo; I updated commit sha1. New commits:
7603fe0 | Implementing Jeroen's (in person) comments.
|
comment:8 Changed 5 years ago by
- Reviewers set to Jeroen Demeyer
comment:9 Changed 5 years ago by
- Commit changed from 7603fe056c0230608dfb2a7e6b7adffbfea88b66 to b7e2e5d604e7d15acf3d3bf666da17308efca79e
Branch pushed to git repo; I updated commit sha1. New commits:
b7e2e5d | Minor fixes to lcm()
|
comment:10 Changed 5 years ago by
Minor changes, needs review.
comment:11 Changed 5 years ago by
If all tests pass, then positive review.
comment:12 Changed 5 years ago by
- Commit changed from b7e2e5d604e7d15acf3d3bf666da17308efca79e to 40e2b0835e257b55aa013d5583ef0f1121111ba1
comment:13 Changed 5 years ago by
- Status changed from needs_review to positive_review
I make 2 very small corrections which were needed for the patchbot.
comment:14 Changed 5 years ago by
Thank you for the review.
comment:15 Changed 5 years ago by
- Branch changed from public/arith/fix_lcm-22630 to 40e2b0835e257b55aa013d5583ef0f1121111ba1
- Resolution set to fixed
- Status changed from positive_review to closed
New timings:
Old timings:
So this does have a constant time slowdown when looping over (Sage) integer lists of about 6/100 µs or 60 ns per call. I think this is acceptable in comparison to how much we gain for a mixed list, fixing the bug at hand, and consolidating things into a single function.
New commits:
Fixing bug in LCM_list and combining with __LCM_sequence.