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: |
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)
Change History (13)
comment:1 Changed 14 years ago by
- Owner changed from somebody to gfurnish
- Status changed from new to assigned
comment:2 Changed 14 years ago by
- Cc robertwb added
comment:3 Changed 14 years ago by
- 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
-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
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
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
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
comment:8 Changed 14 years ago by
- 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
- 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
- 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
- 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
- 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