Ticket #6083 (closed defect: fixed)

Opened 16 months ago

Last modified 14 months ago

[with new patch, with positive review] speedup integer division

Reported by: robertwb Owned by: somebody
Priority: major Milestone:
Component: basic arithmetic Keywords:
Cc: Author(s): Robert Bradshaw
Report Upstream: Reviewer(s): Craig Citro
Merged in: sage-4.1.rc0 Work issues:

Description

remove _sig_on and _sig_off for small operands, specialize for int divisor

Attachments

6083-integer-div.patch Download (6.7 KB) - added by robertwb 16 months ago.
trac-6083-referee.patch Download (6.1 KB) - added by craigcitro 15 months ago.

Change History

Changed 16 months ago by robertwb

Changed 15 months ago by fredrik.johansson

I now get one trivial test failure (changed exception message):

File "/home/fredrik/sage-4.0/devel/sage-mpmath/sage/rings/integer.pyx", line 2163:
    sage: z % 0
Expected:
    Traceback (most recent call last):
    ...
    ZeroDivisionError: Integer modulo by zero
Got:
    Traceback (most recent call last):
      File "/space/wstein/farm/sage-4.0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/space/wstein/farm/sage-4.0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/space/wstein/farm/sage-4.0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_46[4]>", line 1, in <module>
        z % Integer(0)###line 2163:
    sage: z % 0
      File "integer.pyx", line 2189, in sage.rings.integer.Integer.__mod__ (sage/rings/integer.c:15033)
    ZeroDivisionError: other must be nonzero

Otherwise, this patch has my approval.

Changed 15 months ago by robertwb

Thanks for looking at these. I'll fix this (the original error seems better).

Changed 15 months ago by craigcitro

  • summary changed from speedup integer division to [with patch, with positive review modulo referee's patch] speedup integer division

This patch looks good. I've added a referee patch that makes a few really minor changes:

  • removes the unused _floordiv function
  • changes the error messages: they all now say either "Integer division by zero" or "Integer modulo by zero." I think these are more informative, and they also now mirror the Python ones (which all say "integer division or modulus by zero"). The old ones were of the form "other (=%s) must be nonzero"%other, and by definition always had other equal to 0, so that just seemed silly.

Unless Robert or Fredrik has any objections to the second patch, I'd say this is good to go.

Changed 15 months ago by robertwb

  • summary changed from [with patch, with positive review modulo referee's patch] speedup integer division to [with patch, with positive review] speedup integer division

Yep, looks good.

Changed 15 months ago by ncalexan

  • summary changed from [with patch, with positive review] speedup integer division to [with patch, needs work] speedup integer division

Unfortunately, this causes a segfault with the 4.0.2.alpha0 singular:

----------------------------------------------------------------------

The following tests failed:

        sage -t -long devel/sage/sage/rings/polynomial/multi_polynomial_ideal.py # Segfault
----------------------------------------------------------------------
Total time for all tests: 524.4 seconds
Tests failed!

Changed 15 months ago by craigcitro

Changed 15 months ago by craigcitro

  • summary changed from [with patch, needs work] speedup integer division to [with new patch, needs review] speedup integer division

It turns out the segfault was coming from an infinite loop in Cython. The issue was that after the first patch above, doing Integer % IntegerMod_gmp would call into the __mod__ on IntegerMod_gmp, which tried to check if something was zero by doing something of the form Integer % IntegerMod_gmp ... and repeat ad infinitum.

So the new patch adds a small snippet to fix this, and a doctest.

Changed 14 months ago by cremona

  • summary changed from [with new patch, needs review] speedup integer division to [with new patch, with positive review] speedup integer division

Although I have not been following all the details of this one, I applied all patches successfully to 4.1.alpha2 and ran -testall successfully, so I'm giving it a positive review.

Changed 14 months ago by rlm

  • status changed from new to closed
  • reviewer set to Craig Citro
  • resolution set to fixed
  • merged set to sage-4.1.rc0
  • author set to Robert Bradshaw
Note: See TracTickets for help on using tickets.