Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#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 aapitzsch)

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)

trac-10596.patch (42.7 KB) - added by spancratz 9 years ago.
trac-10596-461rc0.patch (42.9 KB) - added by spancratz 9 years ago.
Version for 4.6.1.rc0
trac_10596.patch (41.2 KB) - added by aapitzsch 8 years ago.
trac_10596_remove_trailing_whitespaces.patch (19.2 KB) - added by aapitzsch 8 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 9 years ago by spancratz

  • Description modified (diff)

Changed 9 years ago by spancratz

comment:2 Changed 9 years ago by spancratz

  • Description modified (diff)
  • Status changed from new to needs_review

comment:3 Changed 9 years ago by spancratz

  • Priority changed from major to trivial

comment:4 Changed 9 years ago by spancratz

  • Summary changed from Misc improvements to arithmetic to Misc improvements to integer.pyx

comment:5 follow-up: Changed 9 years ago by jthurber

  • Cc jthurber added

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

comment:6 in reply to: ↑ 5 Changed 9 years ago by aly.deines

  • Status changed from needs_review to needs_work

I had the same thing happen as well.

comment:7 Changed 9 years ago by aly.deines

  • Reviewers set to Aly Deines

comment:8 Changed 9 years ago by spancratz

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

Changed 9 years ago by spancratz

Version for 4.6.1.rc0

comment:9 Changed 9 years ago by aly.deines

  • 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 aapitzsch

integer.pyx: ndigits(): unsigned long b is never used. (rc0 patch version)

comment:11 follow-up: Changed 9 years ago by davidloeffler

  • Authors changed from spancratz to Sebastian Pancratz

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 mstreng

  • 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 aapitzsch

  • 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 aapitzsch

  • Description modified (diff)

comment:15 Changed 8 years ago by zimmerma

  • Cc zimmerma added

Changed 8 years ago by aapitzsch

Changed 8 years ago by aapitzsch

comment:16 Changed 8 years ago by aapitzsch

  • Description modified (diff)

I removed the ndigits() part because problem was fixed in #11796.

comment:17 follow-up: Changed 8 years ago by cremona

  • 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: Changed 8 years ago by cremona

  • 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 cremona

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 aapitzsch

  • 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 aapitzsch

comment:22 in reply to: ↑ 18 Changed 8 years ago by mstreng

  • Status changed from needs_review to positive_review

Replying to cremona:

I would give a positive review were it not for those warnings.

And #11761 shows that the warnings are ok.

comment:23 Changed 8 years ago by aapitzsch

  • Authors changed from Sebastian Pancratz to Sebastian Pancratz, André Apitzsch

comment:24 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-5.0

comment:25 Changed 8 years ago by jdemeyer

  • 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 davidloeffler

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)

Note: See TracTickets for help on using tickets.