#10596 closed enhancement (fixed)
Misc improvements to integer.pyx
Reported by: | spancratz | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | trivial | Milestone: | sage-5.0 |
Component: | basic arithmetic | Keywords: | |
Cc: | jthurber, zimmerma | Merged in: | sage-5.0.beta0 |
Authors: | Sebastian Pancratz, André Apitzsch | Reviewers: | Aly Deines, John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Generic code clean-up such as line breaks, empty lines, use of GMP functions etc
Before:
sage: x = factorial(2**14) sage: timeit('y = odd_part(x)') 625 loops, best of 3: 10.6 µs per loop
sage: odd_part(0) --------------------------------------------------------------------------- ... TypeError: unsupported operands for >>: 0, +Infinity
After:
sage: x = factorial(2**14) sage: timeit('y = x.odd_part()') 625 loops, best of 3: 4.52 µs per loop
sage: ZZ(0).odd_part() 0
Apply trac_10596.patch, trac_10596_remove_trailing_whitespaces.patch
Attachments (4)
Change History (30)
comment:1 Changed 9 years ago by
- Description modified (diff)
Changed 9 years ago by
comment:2 Changed 9 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:3 Changed 9 years ago by
- Priority changed from major to trivial
comment:4 Changed 9 years ago by
- Summary changed from Misc improvements to arithmetic to Misc improvements to integer.pyx
comment:5 follow-up: ↓ 6 Changed 9 years ago by
- Cc jthurber added
comment:6 in reply to: ↑ 5 Changed 9 years ago by
- Status changed from needs_review to needs_work
I had the same thing happen as well.
comment:7 Changed 9 years ago by
- Reviewers set to Aly Deines
comment:8 Changed 9 years ago by
I'm sorry, I think this is because I was working on this with 4.6.0 rather than 4.6.1.rc0. I will fix that this morning. Sebastian
comment:9 Changed 9 years ago by
- Status changed from needs_work to needs_review
All tests pass for me.
There's some of code I don't understand . . . so I'm not giving this a positive review.
comment:10 Changed 9 years ago by
integer.pyx: ndigits()
: unsigned long b is never used. (rc0 patch version)
comment:11 follow-up: ↓ 12 Changed 9 years ago by
Hi Sebastian,
Hope you're well. Trivial comment: it's the done thing to put full names, not trac usernames, in the Author and Reviewer fields because they're used for compiling the release notes.
Less trivial: can you perhaps do a micro-patch that gets rid of the unused variable in ndigits? The rest of the code looks fine to me, and it would be good to get this positively reviewed soon, because any patch that changes quite so many lines of code is going to be highly vulnerable to bitrotting (it already conflicts with my patch at #10625, sigh).
Regards, David
comment:12 in reply to: ↑ 11 Changed 9 years ago by
- Status changed from needs_review to needs_work
Replying to davidloeffler:
it would be good to get this positively reviewed soon, because any patch that changes quite so many lines of code is going to be highly vulnerable to bitrotting
Bitrotting happened: failed to apply trac-10596-461rc0.patch on sage-4.6.2.alpha4
comment:13 Changed 8 years ago by
- Status changed from needs_work to needs_review
Rebased the patch by spancratz and added a patch that removes some trailing whitespaces.
Apply trac_10596_remove_trailing_whitespaces.patch after trac_10596.patch
comment:14 Changed 8 years ago by
- Description modified (diff)
comment:15 Changed 8 years ago by
- Cc zimmerma added
Changed 8 years ago by
Changed 8 years ago by
comment:16 Changed 8 years ago by
- Description modified (diff)
I removed the ndigits() part because problem was fixed in #11796.
comment:17 follow-up: ↓ 18 Changed 8 years ago by
- Status changed from needs_review to needs_info
The patches apply cleanly to 4.8.alpha4: but there are quite a few warning on rebuilding, about variables being referenced before assignment. Does this matter?
comment:18 in reply to: ↑ 17 ; follow-ups: ↓ 19 ↓ 22 Changed 8 years ago by
- Reviewers changed from Aly Deines to Aly Deines, John Cremona
Replying to cremona:
The patches apply cleanly to 4.8.alpha4: but there are quite a few warning on rebuilding, about variables being referenced before assignment. Does this matter?
All tests pass and docbuild is clean. I would give a positive review were it not for those warnings. If someone knows that they are not serious, please tell me.
comment:19 in reply to: ↑ 18 Changed 8 years ago by
Replying to cremona:
Replying to cremona:
The patches apply cleanly to 4.8.alpha4: but there are quite a few warning on rebuilding, about variables being referenced before assignment. Does this matter?
All tests pass and docbuild is clean. I would give a positive review were it not for those warnings. If someone knows that they are not serious, please tell me.
The warnings seem not to be caused by this patch at since when I popped the patch and rebuilt they came up again. So, do they matter?
comment:20 Changed 8 years ago by
- Status changed from needs_info to needs_review
Warnings are caused by lines like
cdef mpz_t tmp mpz_init(tmp)
This seems to be a Cython problem, see http://trac.cython.org/cython_trac/ticket/714 and http://mail.python.org/pipermail/cython-devel/2011-September/001437.html, so warnings shouldn't matter.
comment:21 Changed 8 years ago by
See also comment:17:ticket:11761.
comment:22 in reply to: ↑ 18 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:23 Changed 8 years ago by
comment:24 Changed 8 years ago by
- Milestone changed from sage-4.8 to sage-5.0
comment:25 Changed 8 years ago by
- Merged in set to sage-5.0.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:26 Changed 8 years ago by
Apply trac_10596.patch, trac_10596_remove_trailing_whitespaces.patch
(for the patchbot, so it understands the prerequisites for building #12116 against Sage 4.8)
I had the following failures to apply, 4.6.1.rc1 on OS X. This doesn't have 5945 as a prerequisite, does it?