Opened 10 years ago
Closed 7 years ago
#14304 closed enhancement (fixed)
New implementation of unramified padics using FLINT and templates
Reported by:  David Roe  Owned by:  David Roe 

Priority:  major  Milestone:  sage7.2 
Component:  padics  Keywords:  sd59, days71 
Cc:  Fredrik Johansson, JeanPierre Flori, Julian Rüth  Merged in:  
Authors:  Julian Rüth, David Roe  Reviewers:  David Roe, Julian Rüth, Aly Deines 
Report Upstream:  N/A  Work issues:  
Branch:  56b357c (Commits, GitHub, GitLab)  Commit:  56b357c3f9b59853ec0ed7885a249b120ce58f95 
Dependencies:  #20210  Stopgaps: 
Description
Reimplement Qq using FLINT.
Change History (108)
comment:1 Changed 10 years ago by
Cc:  Fredrik Johansson added 

comment:2 Changed 10 years ago by
Cc:  JeanPierre Flori added 

comment:4 Changed 10 years ago by
Cc:  Julian Rüth added 

comment:6 Changed 9 years ago by
The repository moved to https://github.com/saraedum/sagerenamed/tree/Zq
comment:7 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:8 Changed 9 years ago by
Branch:  → u/saraedum/Zq 

Commit:  → ed8db422d4707069b20cde96e8d7013112aa7591 
Last 10 new commits:
ed8db42  fixup

244b3a8  conversion from finite fields

8f255a9  fix some typos

65d3c03  added matrix_mod_pn

ff52ce0  removed FLINT skip

7b2ad9a  fixed a doctest

fd2ea9f  added flint rep

13a2d60  added an _new_fmpz_poly method

52010f3  switched to powcomputer_

02ffe57  disabled tests for FLINT

comment:9 Changed 9 years ago by
Branch:  u/saraedum/Zq → u/roed/ticket/14304 

Created:  Mar 19, 2013, 3:14:00 AM → Mar 19, 2013, 3:14:00 AM 
Modified:  Jan 5, 2014, 12:38:49 AM → Jan 5, 2014, 12:38:49 AM 
comment:10 Changed 9 years ago by
Authors:  → Julian Rueth, David Roe 

Commit:  ed8db422d4707069b20cde96e8d7013112aa7591 → e7afa8f9e65a29c5097031985248cae09f0483f4 
Reviewers:  → David Roe, Julian Rueth 
Status:  new → needs_review 
comment:11 Changed 9 years ago by
Modified:  Jan 5, 2014, 8:49:26 PM → Jan 5, 2014, 8:49:26 PM 

Status:  needs_review → positive_review 
comment:12 Changed 9 years ago by
I mostly looked at the last two commits. Everything else had already been reviewed before. I did not run doctests. Is there a working patchbot which will run the doctests or do I have to run them manually?
comment:15 Changed 9 years ago by
Commit:  e7afa8f9e65a29c5097031985248cae09f0483f4 → 98a4529906267f0ece2f2a111fc2767ebdd8061d 

Branch pushed to git repo; I updated commit sha1. New commits:
98a4529  Fix doctest errors

comment:16 Changed 9 years ago by
Status:  needs_work → needs_review 

comment:17 followup: 19 Changed 9 years ago by
Status:  needs_review → needs_work 

There is invalid use of sig_on()
in many places. The following is wrong:
sig_on() if condition: raise Exception sig_off()
You must put a sig_off()
before the raise
(or use try
/finally
).
I also recommend to doctest these exceptions. The doctesting framework catches such invalid use of sig_on()
.
I also don't like bare except:
statements. Be explicit and use except BaseException:
or except Exception:
instead.
comment:18 Changed 9 years ago by
Commit:  98a4529906267f0ece2f2a111fc2767ebdd8061d → d0baddeae085ce6aabd34178aceb98a83e7673e5 

Branch pushed to git repo; I updated commit sha1. New commits:
d0badde  Fixing sig_on/sig_off pairs, adding BaseException to bare except:s

comment:19 followup: 21 Changed 9 years ago by
Status:  needs_work → needs_review 

Replying to jdemeyer:
There is invalid use of
sig_on()
in many places. The following is wrong:sig_on() if condition: raise Exception sig_off()You must put a
sig_off()
before theraise
(or usetry
/finally
).
Thanks. I fixed them.
I also recommend to doctest these exceptions. The doctesting framework catches such invalid use of
sig_on()
.
All of the instances I found were for problems that we never expect to arise (out of memory errors, etc). So I don't see a reasonable way to doctest these exceptions.
I also don't like bare
except:
statements. Be explicit and useexcept BaseException:
orexcept Exception:
instead.
I know. :) I told Julian you didn't like them, but he claimed it was okay since we raised after catching in each case. Anyway, I've changed them to except BaseException:
.
comment:20 followup: 25 Changed 9 years ago by
For the record, the Python docs say
exception BaseException
The base class for all builtin exceptions. It is not meant to be directly inherited by userdefined classes (for that, use Exception).
Nested exception blocks are generally hard to read, quadruply nested exception try/except/finally blocks even more so. Besides the usual advice of breaking long functions up into shorter ones, for the heap cleanup in the face of exceptions make sure that invalid / not yet allocated pointers are NULL. To simplify the cleanup part, free(NULL)
is a noop.
The for i from 0 <= i <= n
construct is old, Cython nowadays produces the same C code for the (preferred) Python syntax for i in range(n)
.
comment:21 Changed 9 years ago by
Replying to roed:
but he claimed it was okay since we raised after catching in each case. Anyway, I've changed them to
except BaseException:
.
It is technically ok in this case. The reason I don't like except:
is that in almost all cases that I have seen, it was a mistake. If you write except BaseException:
, it shows that you probably know what you're doing and that it is not a mistake. It is also consistent with "Explicit is better than implicit" from the Zen of Python (import this
).
comment:22 Changed 9 years ago by
sig_on()
should be outside the try/finally block (if sig_on()
raises a KeyboardInterrupt
, then sig_off()
should not be called)
sig_on() try: ... finally: sig_off()
comment:23 Changed 9 years ago by
Status:  needs_review → needs_work 

comment:24 Changed 9 years ago by
Commit:  d0baddeae085ce6aabd34178aceb98a83e7673e5 → c647f4a1413547a47196f77acade2d7c45b46c28 

comment:25 followup: 27 Changed 9 years ago by
Nested exception blocks are generally hard to read, quadruply nested exception try/except/finally blocks even more so. Besides the usual advice of breaking long functions up into shorter ones, for the heap cleanup in the face of exceptions make sure that invalid / not yet allocated pointers are NULL. To simplify the cleanup part,
free(NULL)
is a noop.
What about things like mpz_clear
, fmpz_clear
and fmpz_poly_clear
? If a
is an fmpz_poly_t
that has not yet had fmpz_poly_init(a)
run on it, won't fmpz_poly_clear(a)
be a double free?
comment:26 Changed 9 years ago by
Status:  needs_work → needs_review 

I've addressed the concerns about sig_on
and the Cython for loops. I agree that the supernested try blocks are ugly, but I don't know how to rewrite it safely without this nesting.
comment:27 followup: 28 Changed 9 years ago by
Replying to roed:
What about things like
mpz_clear
,fmpz_clear
andfmpz_poly_clear
? Ifa
is anfmpz_poly_t
that has not yet hadfmpz_poly_init(a)
run on it, won'tfmpz_poly_clear(a)
be a double free?
Yes, they will and Sage will crash, or at least could. On Linux you can set _MALLOC_CHECK=3 to ensure (or have a higher probability) it will crash in such cases.
comment:28 followup: 30 Changed 9 years ago by
Replying to jpflori:
Replying to roed:
What about things like
mpz_clear
,fmpz_clear
andfmpz_poly_clear
? Ifa
is anfmpz_poly_t
that has not yet hadfmpz_poly_init(a)
run on it, won'tfmpz_poly_clear(a)
be a double free?Yes, they will and Sage will crash, or at least could. On Linux you can set _MALLOC_CHECK=3 to ensure (or have a higher probability) it will crash in such cases.
So, does anyone have suggestions on how to rewrite the ugly nested try blocks in PowComputer_flint_unram.__cinit__
? Julian and I couldn't think of a better way to do it that would never have a memory leak or a double free.
comment:30 followup: 31 Changed 9 years ago by
Replying to roed:
So, does anyone have suggestions on how to rewrite the ugly nested try blocks in
PowComputer_flint_unram.__cinit__
? Julian and I couldn't think of a better way to do it that would never have a memory leak or a double free.
I had a look at the code and the current code actually is never going to work. The Clevel functions like fmpz_poly_init
and mpz_init
never raise exceptions. So, the try
/except
is pointless. As far as I can tell, there are two kinds of exceptions which can happen: a MemoryError
in MPIR or a KeyboardInterrupt
. In both cases, the exception will be raised by sig_on()
and none of the except
blocks are executed.
If you want to be absolutely correct, every single allocation should be wrapped individually in sig_on()
/sig_off()
. Example for 2 allocations:
sig_on() alloc1() sig_off() try: sig_on() alloc2() sig_off() except BaseException: free1() raise
But at least the very deeply nested block on this ticket is allocating extremely little memory (surely less than 1000 bytes), so why not simply
alloc1() alloc2()
comment:31 followup: 32 Changed 9 years ago by
Replying to jdemeyer:
Replying to roed:
So, does anyone have suggestions on how to rewrite the ugly nested try blocks in
PowComputer_flint_unram.__cinit__
? Julian and I couldn't think of a better way to do it that would never have a memory leak or a double free.I had a look at the code and the current code actually is never going to work. [...]
You are right. I had misunderstood how sig_on
/sig_off
worked.
But at least the very deeply nested block on this ticket is allocating extremely little memory (surely less than 1000 bytes), so why not simply
alloc1() alloc2()
Is this acceptable? In the unlikely case that this allocation fails because we're out of memory, then sage is going to crash right?
comment:32 Changed 9 years ago by
Replying to saraedum:
Is this acceptable? In the unlikely case that this allocation fails because we're out of memory, then sage is going to crash right?
Well, at least the very deeply nested allocation in this branch is allocating very little memory. If that already fails, bad things are going to happen anyway.
comment:33 Changed 9 years ago by
Commit:  c647f4a1413547a47196f77acade2d7c45b46c28 → 60975a4eed6b9bc665219709e337f801eb2b264d 

comment:34 Changed 9 years ago by
Here are two commits; the first has the 11nested try blocks with "safe" memory handling, and the second removes them for code readability. I'm fine with either approach.
comment:35 followup: 36 Changed 9 years ago by
This comment is wrong:
While the following allocations have the potential to leak small amounts of memory when interrupted or when one of the init methods raises a MemoryError
If you don't have sig_on()
, the code cannot be interrupted (unless it's called from within sig_on()
, but that would not be a good idea). It also cannot raise a MemoryError
, since there is no sig_on()
to raise them. Instead of a MemoryError
, Sage would simply crash hard.
comment:36 followup: 37 Changed 9 years ago by
Replying to jdemeyer:
If you don't have
sig_on()
, the code cannot be interrupted (unless it's called from withinsig_on()
, but that would not be a good idea). It also cannot raise aMemoryError
, since there is nosig_on()
to raise them. Instead of aMemoryError
, Sage would simply crash hard.
So the whole block of allocations should be wrapped in a single sig_on()
/sig_off()
?
comment:37 Changed 9 years ago by
Replying to roed:
So the whole block of allocations should be wrapped in a single
sig_on()
/sig_off()
?
In that case, memory leaks are possible and the comment would be 100% correct.
comment:38 followup: 39 Changed 9 years ago by
I see that there is some smartness added to sage_mpir_malloc/... functions in src/c_lib/memory.c, and these functions are hopefully called when mpir_init is (instead of plain malloc/...).
But as far as I now, nothing similar was done for flint, and if that's the case, a failed memory allocation would go undetected anyway (and Sage crash quite quickly with null pointer dereference or similar delicious stuff).
I just looked at flint memory allocation code (flfint_malloc/...), and a failed allocation by malloc would call the flint_memory_error function which calls abort().
comment:39 Changed 9 years ago by
Replying to jpflori:
I just looked at flint memory allocation code (flfint_malloc/...), and a failed allocation by malloc would call the flint_memory_error function which calls abort().
That's great because (unlike MPIR which silenty ignores memory errors) an abort()
within sig_on()
yields a RuntimeError
which can safely be handled.
comment:40 Changed 9 years ago by
Why not increment the __allocated
counter after every successful allocation and use its value in __dealloc__()
to decide which objects have to be deallocated? E.g.
def __cinit__(self, ...): sig_on() self.__allocated = 0 fmpz_init(self.fmpz_ccmp); self.__allocated++ fmpz_init(self.fmpz_cval); self.__allocated++ ... fmpz_poly_init(self.poly_cconv); self.__allocated++ fmpz_poly_init(self.poly_ctm); self.__allocated++ ... sig_off() def __dealloc__(self, ...): if self.__allocated >= 1: fmpz_clear(self.fmpz_ccmp) if self.__allocated >= 2: fmpz_clear(self.fmpz_cval) ...
I hope I'm understanding the problem correctly; I'm just looking at the code and didn't follow the above discussion very closely.
comment:41 followup: 42 Changed 9 years ago by
For that to work properly, self.__allocated
should have type volatile int
.
comment:42 followup: 43 Changed 9 years ago by
Replying to jdemeyer:
For that to work properly,
self.__allocated
should have typevolatile int
.
OK, I can believe that. Does Cython support volatile
?
comment:43 Changed 9 years ago by
Replying to pbruin:
Replying to jdemeyer:
For that to work properly,
self.__allocated
should have typevolatile int
.OK, I can believe that. Does Cython support
volatile
?
Not directly, as far as I know. You can do
cdef extern from *: ctypedef int volatile_int "volatile int"
and then use volatile_int
.
comment:44 Changed 9 years ago by
Agree with the volatile being necessary so that the optimizer doesn't combine the increments into a single += n. But afaik it would still be legal for the optimizer to reorder statements as
self.__allocated++ self.__allocated++ ... self.__allocated++ fmpz_init(self.fmpz_ccmp) fmpz_init(self.fmpz_cval) ... fmpz_poly_init(self.poly_ctm)
So either mark everything that needs to be accessed inorder as volatile. Or use the usual pattern
def __cinit__(self, ...): self.fmpz_ccmp = NULL self.fmpz_cval = NULL sig_on() fmpz_init(self.fmpz_ccmp) fmpz_init(self.fmpz_cval) sig_off() def __dealloc__(self, ...): if self.fmpz_ccmp != NULL: fmpz_clear(self.fmpz_ccmp) if self.fmpz_ccval != NULL: fmpz_clear(self.fmpz_ccval)
comment:45 Changed 9 years ago by
I guess that can still be broken by creative reordering of the pointer initialization, so to be 100% safe one would need to split the setting to NULL from the fmpz_init
either by
 a compiler barrier (
asm volatile("" ::: "memory")
in gcc)  move the
fmpz_init
into__init__
 RAII (separate classes for each
fmpz_poly_t
)
comment:46 Changed 9 years ago by
Commit:  60975a4eed6b9bc665219709e337f801eb2b264d → cda1d750c9d7930b5997e036dd5a01a5398bbbbb 

Branch pushed to git repo; I updated commit sha1. New commits:
cda1d75  Wrap allocations in sig_on/sig_off

comment:47 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:48 Changed 9 years ago by
Status:  needs_review → needs_work 

I merged the latest development branch (conflicts in sage/rings/padics/factory.py
and sage/rings/padics/pow_computer.pyx
), and then got this error while compiling:
Error compiling Cython file:  ... fmpz_poly_content(prime_pow.fmpz_cinv, a) fmpz_poly_scalar_divexact_fmpz(out, a, prime_pow.fmpz_cinv) fmpz_poly_xgcd(prime_pow.fmpz_cinv2, out, prime_pow.poly_cinv2, out, prime_pow.poly_cinv) if fmpz_is_zero(prime_pow.cinv2): raise ValueError("polynomials are not coprime") ^  ./sage/libs/linkages/padics/fmpz_poly_unram.pxi:343:33: Cannot convert Python object to 'fmpz_t'
comment:49 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:50 Changed 8 years ago by
Branch:  u/roed/ticket/14304 → u/saraedum/ticket/14304 

Modified:  May 6, 2014, 3:20:58 PM → May 6, 2014, 3:20:58 PM 
comment:51 Changed 8 years ago by
Keywords:  sd59 added 

Status:  needs_work → needs_review 
comment:52 Changed 8 years ago by
Commit:  cda1d750c9d7930b5997e036dd5a01a5398bbbbb → c4348c282371ba0e8b53eb83f3ee0c10fd05d1e2 

I'm happy with the changes roed made since my last positive_review
. So from my point of view this could go back to positive_review
once somebody reviewed my few changes and if the patchbot is happy.
New commits:
f11fb44  Merge branch 'develop' into ticket/14304

cce5076  adapt to the new version of FLINT's padic_ctx_init

994be79  Fixed a typo in linkage to FLINT for qadics

c4348c2  fixed a doctest in padic factor

comment:53 Changed 8 years ago by
Commit:  c4348c282371ba0e8b53eb83f3ee0c10fd05d1e2 → 5b42f9bfb4db5cd99a749a17ba283feaa215abdc 

comment:55 Changed 8 years ago by
Commit:  5b42f9bfb4db5cd99a749a17ba283feaa215abdc → f33bd5bc704476a03c99f46156b046fa88b5e547 

comment:56 Changed 8 years ago by
Commit:  f33bd5bc704476a03c99f46156b046fa88b5e547 → c7ffd25e860575755421c503a3189fbd928a0ab5 

Branch pushed to git repo; I updated commit sha1. New commits:
c7ffd25  Fix copy() for qadic conversion maps to capped absolute rings

comment:57 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:59 Changed 7 years ago by
Commit:  c7ffd25e860575755421c503a3189fbd928a0ab5 → 9fecbdd438ecede712a97afe4edb2242eeb25cb0 

Branch pushed to git repo; I updated commit sha1. New commits:
9fecbdd  Merge branch 'develop' into t/14304/ticket/14304

comment:60 Changed 7 years ago by
Commit:  9fecbdd438ecede712a97afe4edb2242eeb25cb0 → 5ba4124ac05a5587a9c9dfc1be19f831d3a45874 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5ba4124  Merge branch 'develop' into t/14304/ticket/14304

comment:61 Changed 7 years ago by
Commit:  5ba4124ac05a5587a9c9dfc1be19f831d3a45874 → 5e17cdba912143101d8c32a5a8c1613366c9d49d 

comment:62 Changed 7 years ago by
Commit:  5e17cdba912143101d8c32a5a8c1613366c9d49d → 2c3a71063fd13752a1f839a573191139fb1fc9ea 

Branch pushed to git repo; I updated commit sha1. New commits:
2c3a710  implement _cache_key for qadics

comment:63 followup: 85 Changed 7 years ago by
Dependencies:  #12555, #12173 

Just a few technical comments (I don't plan to formally review this):
 You don't need to add
include_dirs = [SAGE_INC + '/flint']
(just addflint/
to the path to the.h
files)  You don't need to add
__richcmp__
calling_richcmp
 You don't need to add
__hash__
which just raisesTypeError
: allElement
s are now unhashable by default.  For consistency with other
.pxd
files, could you move all declarations of FLINT types tosage/libs/flint/types.pxd
?  In this block
try: padic_ctx_init(self.ctx, self.fprime, 0, prec_cap, PADIC_SERIES) except: mpz_clear(self.top_power) raise
given that padic_ctx_init
is a C function, how could it ever raise a Python exception? Moreover, I find it bad style to use except:
, you should be more explicit and catch except BaseException:
instead (the reason I say this is because most people write except:
by mistake).
comment:64 Changed 7 years ago by
Commit:  2c3a71063fd13752a1f839a573191139fb1fc9ea → ab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a 

Branch pushed to git repo; I updated commit sha1. New commits:
ab07bc4  remove unnecessary include_dirs for FLINT from module_list.py

comment:65 Changed 7 years ago by
Commit:  ab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a → 70c780d5c1da3475135cba2db12903b59f1ade1b 

Branch pushed to git repo; I updated commit sha1. New commits:
52425be  Merge branch 'develop' into t/14304/ticket/14304

2a1f293  remove unnecessary implementation of __hash__ and __richcmp__

6fbb0d7  fix conversion from residue field to qadic fields

70c780d  Merge branch 'develop' into t/14304/ticket/14304

comment:66 Changed 7 years ago by
Commit:  70c780d5c1da3475135cba2db12903b59f1ade1b → 0ab73f6de4d5ece9779841971aa33a16894f7c47 

Branch pushed to git repo; I updated commit sha1. New commits:
0ab73f6  Convert to padic residue field if there is a coercion

comment:67 Changed 7 years ago by
Why this change?

src/sage/algebras/quatalg/quaternion_algebra_element.pyx
diff git a/src/sage/algebras/quatalg/quaternion_algebra_element.pyx b/src/sage/algebras/quatalg/quaternion_algebra_element.pyx index 03470ef..c5620e1 100644
a b quaternion algebras and quaternion algebras over number fields. 18 18 # http://www.gnu.org/licenses/ 19 19 #***************************************************************************** 20 20 21 from sage.libs.flint.fmpz cimport * 22 from sage.libs.flint.fmpz_poly cimport * 23 from sage.libs.flint.ntl_interface cimport * 21 24 from sage.structure.element cimport AlgebraElement, RingElement, ModuleElement, Element 22 25 from sage.algebras.quatalg.quaternion_algebra_element cimport QuaternionAlgebraElement_abstract 23 26 from sage.rings.rational cimport Rational
comment:68 Changed 7 years ago by
There are also some strange added imports in src/sage/rings/polynomial/...
comment:69 Changed 7 years ago by
Similarly:

src/sage/libs/flint/ntl_interface.pxd
diff git a/src/sage/libs/flint/ntl_interface.pxd b/src/sage/libs/flint/ntl_interface.pxd index 7cea729..6b88b9b 100644
a b from types cimport fmpz_t, fmpz_poly_t 2 2 3 3 from sage.libs.ntl.ntl_ZZ_decl cimport ZZ_c 4 4 from sage.libs.ntl.ntl_ZZX_decl cimport ZZX_c 5 from sage.libs.flint.fmpz cimport fmpz_t 6 from sage.libs.flint.fmpz_poly cimport fmpz_poly_t 5 7 6 8 cdef extern from "flint/NTLinterface.h": 7 9 void fmpz_poly_get_ZZX(ZZX_c output, fmpz_poly_t poly)
comment:70 Changed 7 years ago by
These imports happened because we fixed flint build errors by importing the necessary symbols. We just imported them in different places than what got into develop did. So the merge did not catch this conflict it seems. Thanks for pointing these out.
comment:71 Changed 7 years ago by
Commit:  0ab73f6de4d5ece9779841971aa33a16894f7c47 → 63d90c692635f9aa1ca2d87cf47f777dc5d6af03 

Branch pushed to git repo; I updated commit sha1. New commits:
63d90c6  remove duplicate imports

comment:72 Changed 7 years ago by
Commit:  63d90c692635f9aa1ca2d87cf47f777dc5d6af03 → 94dda77df5900aa53ae2ffe51259c2b8331d18c5 

comment:73 Changed 7 years ago by
Commit:  94dda77df5900aa53ae2ffe51259c2b8331d18c5 → d2ced66f3fbb8e9955b4d46baf02359b34b0f7f0 

Branch pushed to git repo; I updated commit sha1. New commits:
d2ced66  Fix conversion from finite rings to qadic fields

comment:74 Changed 7 years ago by
Dependencies:  → #19646 

comment:75 Changed 7 years ago by
Conflicts with 6.10.beta6. If you fix this conflict, I can easily merge #19646.
comment:76 Changed 7 years ago by
Commit:  d2ced66f3fbb8e9955b4d46baf02359b34b0f7f0 → 1238235ff417f8c72553419f06d2cef7dd5d40a2 

Branch pushed to git repo; I updated commit sha1. New commits:
1238235  Merge branch 'develop' into t/14304/ticket/14304

comment:77 Changed 7 years ago by
Commit:  1238235ff417f8c72553419f06d2cef7dd5d40a2 → 33df900f7f45683a6e0b23fb91bca43307c1a1fd 

comment:78 Changed 7 years ago by
Commit:  33df900f7f45683a6e0b23fb91bca43307c1a1fd → 73303b8a5aa8027b68d16a01883aefb7f821e609 

comment:79 Changed 7 years ago by
Status:  needs_work → needs_review 

All doctests in rings/
now pass (have not checked with long
.)
comment:80 Changed 7 years ago by
Commit:  73303b8a5aa8027b68d16a01883aefb7f821e609 → 8291b03d4803982922e53573948681ed7d64e9ec 

comment:81 Changed 7 years ago by
Dependencies:  #19646 

Milestone:  sage6.4 → sage7.0 
comment:84 Changed 7 years ago by
Commit:  8291b03d4803982922e53573948681ed7d64e9ec → 4b33f86e947f3ae1b3b35f311bb18f38bb62217c 

comment:85 Changed 7 years ago by
Replying to jdemeyer:
 You don't need to add
include_dirs = [SAGE_INC + '/flint']
(just addflint/
to the path to the.h
files)
Fixed in ab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a.
 You don't need to add
__richcmp__
calling_richcmp
Fixed in 2a1f29345297da21ffe14c47e39faa1ac76eda5a.
 You don't need to add
__hash__
which just raisesTypeError
: allElement
s are now unhashable by default.
I actually do need this implementation to override the base class. Adressed in 2a1f29345297da21ffe14c47e39faa1ac76eda5a.
 For consistency with other
.pxd
files, could you move all declarations of FLINT types tosage/libs/flint/types.pxd
?
I believe that this has happened by now.
 In this block
try: padic_ctx_init(self.ctx, self.fprime, 0, prec_cap, PADIC_SERIES) except: mpz_clear(self.top_power) raisegiven that
padic_ctx_init
is a C function, how could it ever raise a Python exception? Moreover, I find it bad style to useexcept:
, you should be more explicit and catchexcept BaseException:
instead (the reason I say this is because most people writeexcept:
by mistake).
Both fixed in 860366d0bf36e8684c97f5bd5d3e336e79f5795d.
New commits:
e376e21  Merge branch 'develop' into t/14304/ticket/14304

860366d  remove except: for C function call in pow_computer

4b33f86  fix build error

comment:86 Changed 7 years ago by
comment:87 Changed 7 years ago by
Commit:  4b33f86e947f3ae1b3b35f311bb18f38bb62217c → e8c1e41641ced2f346c906c984fb978aa3efc1ce 

Branch pushed to git repo; I updated commit sha1. New commits:
e8c1e41  remove cdefs.pxi imports in qadics

comment:88 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:89 Changed 7 years ago by
Branch:  u/saraedum/ticket/14304 → public/14304 

Commit:  e8c1e41641ced2f346c906c984fb978aa3efc1ce → 87ee786dbea263b0d0e587f5d27fd5f35aa84e62 
comment:90 Changed 7 years ago by
Milestone:  sage7.0 → sage7.2 

comment:91 Changed 7 years ago by
Commit:  87ee786dbea263b0d0e587f5d27fd5f35aa84e62 → 26c8669ca7cffe6b2a271a547e16245a59ede61f 

Branch pushed to git repo; I updated commit sha1. New commits:
c3b41a4  Merge branch 'develop' into t/14303/ticket/14304

2141bd9  Fix bug that would be triggered for ramified extensions

9c4aecb  Improve docstring to use .. WARNING

354bc5e  Break up (some of) the long lines

b422cfc  Merge branch 'public/14304' of git://trac.sagemath.org/sage into t/14304/ticket/14304

26c8669  Fix typo in pow computer

comment:92 Changed 7 years ago by
Status:  needs_review → needs_work 

On line 494 in sage/local/lib/python2.7/sitepackages/sage/rings/padics/factory.py, you need to break the line using parenthesis.
aly@alylaptop:~/Sage/sage$ ./sage t src/sage Traceback (most recent call last): File "/home/aly/Sage/sage/src/bin/sageruntests", line 80, in <module> from sage.doctest.control import DocTestController File "/home/aly/Sage/sage/local/lib/python2.7/sitepackages/sage/doctest/control.py", line 30, in <module> from sources import FileDocTestSource, DictAsObject File "/home/aly/Sage/sage/local/lib/python2.7/sitepackages/sage/doctest/sources.py", line 27, in <module> from parsing import SageDocTestParser File "/home/aly/Sage/sage/local/lib/python2.7/sitepackages/sage/doctest/parsing.py", line 49, in <module> from sage.all import RealIntervalField File "/home/aly/Sage/sage/local/lib/python2.7/sitepackages/sage/all.py", line 100, in <module> from sage.rings.all import * File "/home/aly/Sage/sage/local/lib/python2.7/sitepackages/sage/rings/all.py", line 62, in <module> from padics.all import * File "/home/aly/Sage/sage/local/lib/python2.7/sitepackages/sage/rings/padics/all.py", line 2, in <module> from factory import Zp, Zq, Zp as pAdicRing, ZpCR, ZpCA, ZpFM, ZqCR, ZqCA, ZqFM #, ZpL, ZqL File "/home/aly/Sage/sage/local/lib/python2.7/sitepackages/sage/rings/padics/factory.py", line 494 if version[0] < 4 or (len(version) > 1 and version[0] == 4 and version[1] < 5) or ^ SyntaxError: invalid syntax
comment:93 Changed 7 years ago by
Commit:  26c8669ca7cffe6b2a271a547e16245a59ede61f → baca1928045614982f170e4efc5f56585c223352 

Branch pushed to git repo; I updated commit sha1. New commits:
baca192  Add parentheses for line wraps in factory.pyx

comment:95 Changed 7 years ago by
Dependencies:  → #20210 

Status:  needs_review → needs_work 
Needs to be rebased on top of #20210.
comment:96 Changed 7 years ago by
Commit:  baca1928045614982f170e4efc5f56585c223352 → c22ae81406ebc5b496227a2d2547a89363e4fbfe 

comment:97 Changed 7 years ago by
Commit:  c22ae81406ebc5b496227a2d2547a89363e4fbfe → a14ee5d2d39b3ddd4e8bcbd76922af8bf3aeb213 

Branch pushed to git repo; I updated commit sha1. New commits:
a14ee5d  Fixing imports and name changes due to switch to cysignals

comment:98 Changed 7 years ago by
Okay, I think I rebased it, but there are a ton of doctest errors that have nothing to do with padics. I'm not sure what's going on.
sage t src/sage/combinat/root_system/root_lattice_realizations.py # 1 doctest failed sage t src/sage/combinat/designs/database.py # 2 doctests failed sage t src/sage/geometry/polyhedron/base.py # 2 doctests failed sage t src/sage/graphs/generic_graph.py # 94 doctests failed sage t src/sage/graphs/graph.py # 46 doctests failed sage t src/sage/doctest/forker.py # 1 doctest failed sage t src/sage/schemes/generic/algebraic_scheme.py # 10 doctests failed sage t src/sage/graphs/generators/smallgraphs.py # 18 doctests failed sage t src/sage/tests/cmdline.py # 14 doctests failed sage t src/sage/structure/sage_object.pyx # 4 doctests failed sage t src/sage/combinat/rigged_configurations/rigged_configuration_element.py # 1 doctest failed sage t src/sage/schemes/toric/points.py # 1 doctest failed sage t src/sage/combinat/posets/posets.py # 5 doctests failed sage t src/sage/interfaces/sage0.py # 1 doctest failed sage t src/sage/combinat/designs/orthogonal_arrays.py # 8 doctests failed sage t src/sage/graphs/generators/families.py # 4 doctests failed sage t src/sage/geometry/cone.py # 3 doctests failed sage t src/sage/combinat/rigged_configurations/rigged_configurations.py # 1 doctest failed sage t src/sage/combinat/designs/orthogonal_arrays_build_recursive.py # 13 doctests failed sage t src/sage/matroids/matroid.pyx # 1 doctest failed sage t src/sage/repl/interpreter.py # 22 doctests failed sage t src/sage/structure/misc.pyx # 1 doctest failed sage t src/sage/combinat/designs/incidence_structures.py # 10 doctests failed sage t src/sage/numerical/optimize.py # 6 doctests failed sage t src/sage/combinat/rigged_configurations/rc_infinity.py # 1 doctest failed sage t src/sage/graphs/comparability.pyx # 6 doctests failed sage t src/sage/graphs/generators/basic.py # 1 doctest failed sage t src/sage/repl/ipython_extension.py # 9 doctests failed sage t src/sage/repl/attach.py # 2 doctests failed sage t src/sage/graphs/digraph.py # 4 doctests failed sage t src/sage/typeset/ascii_art.py # 2 doctests failed sage t src/sage/numerical/mip.pyx # 486 doctests failed sage t src/sage/tests/french_book/nonlinear_doctest.py # 1 doctest failed sage t src/sage/interacts/debugger.py # 1 doctest failed sage t src/sage/repl/display/formatter.py # 10 doctests failed sage t src/sage/combinat/designs/bibd.py # 19 doctests failed sage t src/sage/repl/inputhook.pyx # 2 doctests failed sage t src/sage/misc/temporary_file.py # 1 doctest failed sage t src/sage/doctest/test.py # 1 doctest failed sage t src/sage/repl/interface_magic.py # 3 doctests failed sage t src/sage/dev/trac_interface.py # 1 doctest failed sage t src/doc/en/thematic_tutorials/lie/infinity_crystals.rst # 1 doctest failed sage t src/sage/combinat/posets/poset_examples.py # 1 doctest failed sage t src/sage/repl/ipython_tests.py # 4 doctests failed sage t src/sage/combinat/designs/orthogonal_arrays_find_recursive.pyx # 2 doctests failed sage t src/sage/graphs/graph_decompositions/vertex_separation.pyx # 7 doctests failed sage t src/sage/numerical/backends/glpk_backend.pyx # 470 doctests failed sage t src/sage/combinat/integer_vector.py # 5 doctests failed sage t src/sage/numerical/linear_functions.pyx # 212 doctests failed sage t src/sage/combinat/rigged_configurations/rc_crystal.py # 1 doctest failed sage t src/sage/combinat/shard_order.py # 1 doctest failed sage t src/sage/graphs/generators/degree_sequence.py # 3 doctests failed sage t src/sage/repl/rich_output/backend_ipython.py # 3 doctests failed sage t src/sage/numerical/linear_tensor_element.pyx # 50 doctests failed sage t src/doc/en/thematic_tutorials/numerical_sage/weave.rst # 1 doctest failed sage t src/sage/numerical/linear_tensor_constraints.py # 46 doctests failed sage t src/sage/numerical/backends/cvxopt_backend.pyx # 10 doctests failed sage t src/sage/numerical/linear_tensor.py # 57 doctests failed sage t src/sage/repl/ipython_kernel/install.py # 1 doctest failed sage t src/sage/graphs/generators/chessboard.py # 1 doctest failed sage t src/doc/en/reference/sat/index.rst # 4 doctests failed sage t src/sage/numerical/backends/generic_backend.pyx # 1 doctest failed sage t src/sage/graphs/graph_coloring.py # 17 doctests failed sage t src/sage/numerical/knapsack.py # 5 doctests failed sage t src/sage/graphs/hypergraph_generators.py # 1 doctest failed sage t src/sage/repl/ipython_kernel/kernel.py # 4 doctests failed sage t src/doc/en/thematic_tutorials/linear_programming.rst # 35 doctests failed sage t src/sage/sat/solvers/sat_lp.py # 15 doctests failed sage t src/sage/graphs/graph_decompositions/cutwidth.pyx # 9 doctests failed sage t src/sage/graphs/convexity_properties.pyx # 4 doctests failed sage t src/sage/sat/solvers/satsolver.pyx # 1 doctest failed sage t src/sage/numerical/backends/gurobi_backend.pyx # 1 doctest failed
comment:99 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:100 Changed 7 years ago by
Commit:  a14ee5d2d39b3ddd4e8bcbd76922af8bf3aeb213 → 56b357c3f9b59853ec0ed7885a249b120ce58f95 

Branch pushed to git repo; I updated commit sha1. New commits:
56b357c  Fixing some more pxi includes

comment:102 Changed 7 years ago by
I'm getting the same list of doctests failing. They don't fail on #20210. This is weird. I did a clean pull and tested them. None of them seem to have anything to do with your patch. Are they passing for you?
comment:104 Changed 7 years ago by
Status:  needs_review → positive_review 

I updated my development branch and now everything is passing. It looks good to me! :)
comment:105 Changed 7 years ago by
Keywords:  sd71 added 

comment:106 Changed 7 years ago by
Keywords:  days71 added; sd71 removed 

Reviewers:  David Roe, Julian Rueth → David Roe, Julian Rueth, Aly Deines 
comment:107 Changed 7 years ago by
Authors:  Julian Rueth, David Roe → Julian Rüth, David Roe 

Reviewers:  David Roe, Julian Rueth, Aly Deines → David Roe, Julian Rüth, Aly Deines 
(I was told to be consistent about my last name…)
comment:108 Changed 7 years ago by
Branch:  public/14304 → 56b357c3f9b59853ec0ed7885a249b120ce58f95 

Resolution:  → fixed 
Status:  positive_review → closed 
This ticket is essentially complete at https://github.com/saraedum/sage/tree/Zq. Once #12173 is positively reviewed we'll make some patches and put this up for review.