Opened 14 years ago

Closed 14 years ago

#3357 closed enhancement (fixed)

[with new patch, positive review] Refactor pool code in integer.pyx

Reported by: gfurnish Owned by: gfurnish
Priority: critical Milestone: sage-3.0.3
Component: basic arithmetic Keywords:
Cc: robertwb Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

This patch moves some of the setup code from integer.pyx into misc.memory and creates a ext/python_rich_object.pxi file. This patch makes it easy to generalize pools to other classes, and is needed for symbolics.

Attachments (1)

trac_3357.patch (15.4 KB) - added by gfurnish 14 years ago.
new patch

Download all attachments as: .zip

Change History (13)

comment:1 Changed 14 years ago by gfurnish

  • Owner changed from somebody to gfurnish
  • Status changed from new to assigned

comment:2 Changed 14 years ago by gfurnish

  • Cc robertwb added

comment:3 Changed 14 years ago by robertwb

  • Summary changed from [with patch, needs review] Refactor pool code in integer.pyx to [with patch, mixed review] Refactor pool code in integer.pyx
  • You appear to have multiple change-sets in this one patch, which may be problematic for importing (though it worked fine for me).
  • The name "memory.pyx" I would probably call it something like "allocate.pyx" which gives a better impression of what it does.
  • Why are pool_stats, etc. added to sage/misc/memory.pyx, but then commented out?
  • Sage doesn't start up anymore. It's an import error, so it looks like an easy fix, but I'm wary of code that you haven't even tested.

comment:4 Changed 14 years ago by gfurnish

-The changeset issue should work correctly (I just exported two patches directly in a row) -Namechange sounds decent enough -The code to make them work is commented out in all pool allocators, so they are there, but they arn't actually used (in current code or in this one) -Sage starts up for me, and in fact it works better then usual.. What did you apply the patch against?

comment:5 Changed 14 years ago by mabshoff

The problem here is that the old build system does not know about memory.[pyx|lxd] because then I get:

sage -t -long devel/sage/sage/structure/wrapper_parent.pyx
Traceback (most recent call last):
  File "/scratch/mabshoff/release-cycle/sage-3.0.2-vg/tmp/.doctest_wrapper_parent.py", line 2, in <module>
    from sage.all_cmdline import *;
  File "/scratch/mabshoff/release-cycle/sage-3.0.2-vg/local/lib/python2.5/site-packages/sage/all_cmdline.py", line 14, in <module>
    from sage.all import *
  File "/scratch/mabshoff/release-cycle/sage-3.0.2-vg/local/lib/python2.5/site-packages/sage/all.py", line 58, in <module>
    from sage.misc.all       import *         # takes a while
  File "/scratch/mabshoff/release-cycle/sage-3.0.2-vg/local/lib/python2.5/site-packages/sage/misc/all.py", line 76, in <module>
    from functional import (additive_order,
  File "/scratch/mabshoff/release-cycle/sage-3.0.2-vg/local/lib/python2.5/site-packages/sage/misc/functional.py", line 34, in <module>
    from sage.rings.complex_double import CDF
  File "integer.pxd", line 9, in sage.rings.complex_double (sage/rings/complex_double.c:9324)
  File "integer.pyx", line 1, in sage.rings.integer (sage/rings/integer.c:22427)
ImportError: No module named memory

As Robert suggested it might also be a good idea to renames memory.$FOO to allocator.$FOO.

Cheers,

Michael

comment:6 Changed 14 years ago by mabshoff

This fixes at least the build issue:

--- a/setup.py  Sat May 24 16:03:19 2008 -0700
+++ b/setup.py  Wed Jun 04 14:46:10 2008 -0700
@@ -720,6 +720,9 @@ ext_modules = [ \
     Extension('sage.rings.integer',
               sources = ['sage/ext/arith.pyx', 'sage/rings/integer.pyx'],
               libraries=['ntl', 'gmp']), \
+
+    Extension('sage.misc.memory',
+                  sources = ['sage/misc/memory.pyx']), \

     Extension('sage.rings.integer_ring',
               sources = ['sage/rings/integer_ring.pyx'],

Now doctesting & valgrinding ....

Cheers,

Michael

comment:7 Changed 14 years ago by mabshoff

Ok, the whole doctest suite valgrinds clean on sage.math. I will merge this patch provided:

  • it doctests clean on OSX
  • Robert signs off on it
  • memory.[pyx|pxd] gets renamed to allocator.[pyx|pxd]
  • the setup.py issue gets fixed

Cheers,

Michael

Changed 14 years ago by gfurnish

new patch

comment:8 Changed 14 years ago by gfurnish

  • Summary changed from [with patch, mixed review] Refactor pool code in integer.pyx to [with new patch, needs new review] Refactor pool code in integer.pyx

comment:9 Changed 14 years ago by mabshoff

  • Summary changed from [with new patch, needs new review] Refactor pool code in integer.pyx to [with new patch, negative review] Refactor pool code in integer.pyx

Arrg, on OSX this patch causes some doctests to use 100% of the CPU without them making any progress. I have not attempted to debug this, but affected doctests inlcude

  • devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py

but there are others.

Cheers,

Michael

comment:10 Changed 14 years ago by mabshoff

  • Summary changed from [with new patch, negative review] Refactor pool code in integer.pyx to [with new patch, needs review] Refactor pool code in integer.pyx

Ok, as it turns out that tree did not doctest correctly without the patch anyway, so I am building a fresh 3.0.3.a1 tree to test. Apologies for the trouble, looks like I will have to ride on the short bus and wear my helmet ;)

Cheers,

Michael

comment:11 Changed 14 years ago by robertwb

  • Summary changed from [with new patch, needs review] Refactor pool code in integer.pyx to [with new patch, (conditional) positive review] Refactor pool code in integer.pyx

Pending all doctests passing I give this a positive review. There is still too much "manual magic" in the integer.pyx file that I'd like to factor out, but that will be for another patch. Perhaps we could work something out at dev days coming up?

comment:12 Changed 14 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed
  • Summary changed from [with new patch, (conditional) positive review] Refactor pool code in integer.pyx to [with new patch, positive review] Refactor pool code in integer.pyx

Merged in Sage 3.0.3.alpha2 since doctests pass for me

Note: See TracTickets for help on using tickets.