Misc improvements to integer.pyx
sagetrac-spancratz opened this issue · 28 comments
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 attachment: trac_10596.patch, attachment: trac_10596_remove_trailing_whitespaces.patch
CC: @sagetrac-jthurber @zimmermann6
Component: basic arithmetic
Author: Sebastian Pancratz, André Apitzsch
Reviewer: Aly Deines, John Cremona
Merged: sage-5.0.beta0
Issue created by migration from https://trac.sagemath.org/ticket/10596
Description changed:
---
+++
@@ -1,4 +1,4 @@
-Generic code clean-up such as line breaks, empty lines etc
+Generic code clean-up such as line breaks, empty lines, use of GMP functions etc
``n.digits()``
@@ -39,3 +39,30 @@
sage: ZZ(0).nbits()
1
```
+
+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
+```Attachment: trac-10596.patch.gz
Description changed:
---
+++
@@ -3,14 +3,6 @@
``n.digits()``
Before:
-
-```
-sage: n = 10**10000000-10**9999990
-sage: timeit('n.ndigits()')
-625 loops, best of 3: 46.6 µs per loop
-sage: timeit('(-n).ndigits()')
-25 loops, best of 3: 9.04 ms per loop
-```
```
sage: ZZ(0).ndigits()
@@ -22,14 +14,6 @@
```
After:
-
-```
-sage: n = 10**10000000-10**9999990
-sage: timeit('n.ndigits()')
-625 loops, best of 3: 1.02 µs per loop
-sage: timeit('(-n).ndigits()')
-125 loops, best of 3: 4.57 ms per loop
-```
```
sage: ZZ(0).ndigits()I had the following failures to apply, 4.6.1.rc1 on OS X.
This doesn't have 5945 as a prerequisite, does it?
22:14:56johnthurber~/sage/sage-4.6.1.rc1/devel/sage$ hg qpush
applying trac-10596.patch
patching file sage/rings/integer.pyx
Hunk #10 FAILED at 1268
Hunk #37 succeeded at 3276 with fuzz 1 (offset -106 lines).
Hunk #42 FAILED at 3577
Hunk #58 succeeded at 4302 with fuzz 1 (offset -106 lines).
Hunk #62 succeeded at 4643 with fuzz 2 (offset -106 lines).
Hunk #73 FAILED at 5289
3 out of 82 hunks FAILED -- saving rejects to file sage/rings/integer.pyx.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac-10596.patch
I had the same thing happen as well.
Reviewer: Aly Deines
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
Attachment: trac-10596-461rc0.patch.gz
Version for 4.6.1.rc0
All tests pass for me.
There's some of code I don't understand . . . so I'm not giving this a positive review.
integer.pyx: ndigits(): unsigned long b is never used. (rc0 patch version)
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
Changed author from spancratz to Sebastian Pancratz
Replying to @loefflerd:
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
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
Description changed:
---
+++
@@ -50,3 +50,5 @@
sage: ZZ(0).odd_part()
0
```
+
+Apply trac_10596.patch, trac_10596_remove_trailing_whitespaces.patchAttachment: trac_10596.patch.gz
Attachment: trac_10596_remove_trailing_whitespaces.patch.gz
Description changed:
---
+++
@@ -1,28 +1,5 @@
Generic code clean-up such as line breaks, empty lines, use of GMP functions etc
-``n.digits()``
-
-Before:
-
-```
-sage: ZZ(0).ndigits()
-0
-sage: ZZ(0).ndigits(base=2)
-0
-sage: ZZ(0).nbits()
-1
-```
-
-After:
-
-```
-sage: ZZ(0).ndigits()
-1
-sage: ZZ(0).ndigits(base=2)
-1
-sage: ZZ(0).nbits()
-1
-```
Before:
@@ -51,4 +28,4 @@
0
```
-Apply trac_10596.patch, trac_10596_remove_trailing_whitespaces.patch
+**Apply** [attachment: trac_10596.patch](https://github.com/sagemath/sage-prod/files/10651846/trac_10596.patch.gz), [attachment: trac_10596_remove_trailing_whitespaces.patch](https://github.com/sagemath/sage-prod/files/10651847/trac_10596_remove_trailing_whitespaces.patch.gz)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?
Replying to @JohnCremona:
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.
Changed reviewer from Aly Deines to Aly Deines, John Cremona
Replying to @JohnCremona:
Replying to @JohnCremona:
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?
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.
See also #11761 comment:17.
Replying to @JohnCremona:
I would give a positive review were it not for those warnings.
And #11761 shows that the warnings are ok.
Changed author from Sebastian Pancratz to Sebastian Pancratz, André Apitzsch
Merged: sage-5.0.beta0