Opened 9 years ago
Closed 6 years ago
#14304 closed enhancement (fixed)
New implementation of unramified padics using FLINT and templates
Reported by:  roed  Owned by:  roed 

Priority:  major  Milestone:  sage7.2 
Component:  padics  Keywords:  sd59, days71 
Cc:  fredrik.johansson, jpflori, saraedum  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 9 years ago by
 Cc fredrik.johansson added
comment:2 Changed 9 years ago by
 Cc jpflori added
comment:3 followup: ↓ 5 Changed 9 years ago by
comment:4 Changed 9 years ago by
 Cc saraedum added
comment:5 in reply to: ↑ 3 Changed 9 years ago by
comment:6 Changed 8 years ago by
The repository moved to https://github.com/saraedum/sagerenamed/tree/Zq
comment:7 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:8 Changed 8 years ago by
 Branch set to u/saraedum/Zq
 Commit set to 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 8 years ago by
 Branch changed from u/saraedum/Zq to u/roed/ticket/14304
 Created changed from 03/19/13 03:14:00 to 03/19/13 03:14:00
 Modified changed from 01/05/14 00:38:49 to 01/05/14 00:38:49
comment:10 Changed 8 years ago by
 Commit changed from ed8db422d4707069b20cde96e8d7013112aa7591 to e7afa8f9e65a29c5097031985248cae09f0483f4
 Reviewers set to David Roe, Julian Rueth
 Status changed from new to needs_review
comment:11 Changed 8 years ago by
 Modified changed from 01/05/14 20:49:26 to 01/05/14 20:49:26
 Status changed from needs_review to positive_review
comment:12 Changed 8 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:13 Changed 8 years ago by
Does not work... please run "make ptestlong"
comment:15 Changed 8 years ago by
 Commit changed from e7afa8f9e65a29c5097031985248cae09f0483f4 to 98a4529906267f0ece2f2a111fc2767ebdd8061d
Branch pushed to git repo; I updated commit sha1. New commits:
98a4529  Fix doctest errors

comment:16 Changed 8 years ago by
 Status changed from needs_work to needs_review
comment:17 followup: ↓ 19 Changed 8 years ago by
 Status changed from needs_review to 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 8 years ago by
 Commit changed from 98a4529906267f0ece2f2a111fc2767ebdd8061d to 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 in reply to: ↑ 17 ; followup: ↓ 21 Changed 8 years ago by
 Status changed from needs_work to 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 8 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 in reply to: ↑ 19 Changed 8 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 8 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 8 years ago by
 Status changed from needs_review to needs_work
comment:24 Changed 8 years ago by
 Commit changed from d0baddeae085ce6aabd34178aceb98a83e7673e5 to c647f4a1413547a47196f77acade2d7c45b46c28
comment:25 in reply to: ↑ 20 ; followup: ↓ 27 Changed 8 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 8 years ago by
 Status changed from needs_work to 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 in reply to: ↑ 25 ; followup: ↓ 28 Changed 8 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 in reply to: ↑ 27 ; followup: ↓ 30 Changed 8 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:29 Changed 8 years ago by
I'll have a look tomorrow (french time).
comment:30 in reply to: ↑ 28 ; followup: ↓ 31 Changed 8 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 in reply to: ↑ 30 ; followup: ↓ 32 Changed 8 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 in reply to: ↑ 31 Changed 8 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 8 years ago by
 Commit changed from c647f4a1413547a47196f77acade2d7c45b46c28 to 60975a4eed6b9bc665219709e337f801eb2b264d
comment:34 Changed 8 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 8 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 in reply to: ↑ 35 ; followup: ↓ 37 Changed 8 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 in reply to: ↑ 36 Changed 8 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 8 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 in reply to: ↑ 38 Changed 8 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 8 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 8 years ago by
For that to work properly, self.__allocated
should have type volatile int
.
comment:42 in reply to: ↑ 41 ; followup: ↓ 43 Changed 8 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 in reply to: ↑ 42 Changed 8 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 8 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 8 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 8 years ago by
 Commit changed from 60975a4eed6b9bc665219709e337f801eb2b264d to cda1d750c9d7930b5997e036dd5a01a5398bbbbb
Branch pushed to git repo; I updated commit sha1. New commits:
cda1d75  Wrap allocations in sig_on/sig_off

comment:47 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:48 Changed 8 years ago by
 Status changed from needs_review to 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 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:50 Changed 7 years ago by
 Branch changed from u/roed/ticket/14304 to u/saraedum/ticket/14304
 Modified changed from 05/06/14 15:20:58 to 05/06/14 15:20:58
comment:51 Changed 7 years ago by
 Keywords sd59 added
 Status changed from needs_work to needs_review
comment:52 Changed 7 years ago by
 Commit changed from cda1d750c9d7930b5997e036dd5a01a5398bbbbb to 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 7 years ago by
 Commit changed from c4348c282371ba0e8b53eb83f3ee0c10fd05d1e2 to 5b42f9bfb4db5cd99a749a17ba283feaa215abdc
comment:54 Changed 7 years ago by
I hope that this fixes all the bugs the patchbot detected.
comment:55 Changed 7 years ago by
 Commit changed from 5b42f9bfb4db5cd99a749a17ba283feaa215abdc to f33bd5bc704476a03c99f46156b046fa88b5e547
comment:56 Changed 7 years ago by
 Commit changed from f33bd5bc704476a03c99f46156b046fa88b5e547 to 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 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:59 Changed 6 years ago by
 Commit changed from c7ffd25e860575755421c503a3189fbd928a0ab5 to 9fecbdd438ecede712a97afe4edb2242eeb25cb0
Branch pushed to git repo; I updated commit sha1. New commits:
9fecbdd  Merge branch 'develop' into t/14304/ticket/14304

comment:60 Changed 6 years ago by
 Commit changed from 9fecbdd438ecede712a97afe4edb2242eeb25cb0 to 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 6 years ago by
 Commit changed from 5ba4124ac05a5587a9c9dfc1be19f831d3a45874 to 5e17cdba912143101d8c32a5a8c1613366c9d49d
comment:62 Changed 6 years ago by
 Commit changed from 5e17cdba912143101d8c32a5a8c1613366c9d49d to 2c3a71063fd13752a1f839a573191139fb1fc9ea
Branch pushed to git repo; I updated commit sha1. New commits:
2c3a710  implement _cache_key for qadics

comment:63 followup: ↓ 85 Changed 6 years ago by
 Dependencies #12555, #12173 deleted
Just a few technical comments (I don't plan to formally review this):
 You don't need to add
include_dirs = [SAGE_INC + '/flint']
 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 catch be more explicit and catch except BaseException:
instead (the reason I say this is because most people write except:
by mistake).
comment:64 Changed 6 years ago by
 Commit changed from 2c3a71063fd13752a1f839a573191139fb1fc9ea to 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 6 years ago by
 Commit changed from ab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a to 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 6 years ago by
 Commit changed from 70c780d5c1da3475135cba2db12903b59f1ade1b to 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 6 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 6 years ago by
There are also some strange added imports in src/sage/rings/polynomial/...
comment:69 Changed 6 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 6 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 6 years ago by
 Commit changed from 0ab73f6de4d5ece9779841971aa33a16894f7c47 to 63d90c692635f9aa1ca2d87cf47f777dc5d6af03
Branch pushed to git repo; I updated commit sha1. New commits:
63d90c6  remove duplicate imports

comment:72 Changed 6 years ago by
 Commit changed from 63d90c692635f9aa1ca2d87cf47f777dc5d6af03 to 94dda77df5900aa53ae2ffe51259c2b8331d18c5
comment:73 Changed 6 years ago by
 Commit changed from 94dda77df5900aa53ae2ffe51259c2b8331d18c5 to d2ced66f3fbb8e9955b4d46baf02359b34b0f7f0
Branch pushed to git repo; I updated commit sha1. New commits:
d2ced66  Fix conversion from finite rings to qadic fields

comment:74 Changed 6 years ago by
 Dependencies set to #19646
comment:75 Changed 6 years ago by
Conflicts with 6.10.beta6. If you fix this conflict, I can easily merge #19646.
comment:76 Changed 6 years ago by
 Commit changed from d2ced66f3fbb8e9955b4d46baf02359b34b0f7f0 to 1238235ff417f8c72553419f06d2cef7dd5d40a2
Branch pushed to git repo; I updated commit sha1. New commits:
1238235  Merge branch 'develop' into t/14304/ticket/14304

comment:77 Changed 6 years ago by
 Commit changed from 1238235ff417f8c72553419f06d2cef7dd5d40a2 to 33df900f7f45683a6e0b23fb91bca43307c1a1fd
comment:78 Changed 6 years ago by
 Commit changed from 33df900f7f45683a6e0b23fb91bca43307c1a1fd to 73303b8a5aa8027b68d16a01883aefb7f821e609
comment:79 Changed 6 years ago by
 Status changed from needs_work to needs_review
All doctests in rings/
now pass (have not checked with long
.)
comment:80 Changed 6 years ago by
 Commit changed from 73303b8a5aa8027b68d16a01883aefb7f821e609 to 8291b03d4803982922e53573948681ed7d64e9ec
comment:81 Changed 6 years ago by
 Dependencies #19646 deleted
 Milestone changed from sage6.4 to sage7.0
comment:83 followup: ↓ 86 Changed 6 years ago by
Avoid cdefs.pxi
, just cimport
whatever you need.
comment:84 Changed 6 years ago by
 Commit changed from 8291b03d4803982922e53573948681ed7d64e9ec to 4b33f86e947f3ae1b3b35f311bb18f38bb62217c
comment:85 in reply to: ↑ 63 Changed 6 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 in reply to: ↑ 83 Changed 6 years ago by
comment:87 Changed 6 years ago by
 Commit changed from 4b33f86e947f3ae1b3b35f311bb18f38bb62217c to e8c1e41641ced2f346c906c984fb978aa3efc1ce
Branch pushed to git repo; I updated commit sha1. New commits:
e8c1e41  remove cdefs.pxi imports in qadics

comment:88 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:89 Changed 6 years ago by
 Branch changed from u/saraedum/ticket/14304 to public/14304
 Commit changed from e8c1e41641ced2f346c906c984fb978aa3efc1ce to 87ee786dbea263b0d0e587f5d27fd5f35aa84e62
comment:90 Changed 6 years ago by
 Milestone changed from sage7.0 to sage7.2
comment:91 Changed 6 years ago by
 Commit changed from 87ee786dbea263b0d0e587f5d27fd5f35aa84e62 to 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 6 years ago by
 Status changed from needs_review to 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 6 years ago by
 Commit changed from 26c8669ca7cffe6b2a271a547e16245a59ede61f to baca1928045614982f170e4efc5f56585c223352
Branch pushed to git repo; I updated commit sha1. New commits:
baca192  Add parentheses for line wraps in factory.pyx

comment:95 Changed 6 years ago by
 Dependencies set to #20210
 Status changed from needs_review to needs_work
Needs to be rebased on top of #20210.
comment:96 Changed 6 years ago by
 Commit changed from baca1928045614982f170e4efc5f56585c223352 to c22ae81406ebc5b496227a2d2547a89363e4fbfe
comment:97 Changed 6 years ago by
 Commit changed from c22ae81406ebc5b496227a2d2547a89363e4fbfe to 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 6 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 6 years ago by
 Status changed from needs_work to needs_review
comment:100 Changed 6 years ago by
 Commit changed from a14ee5d2d39b3ddd4e8bcbd76922af8bf3aeb213 to 56b357c3f9b59853ec0ed7885a249b120ce58f95
Branch pushed to git repo; I updated commit sha1. New commits:
56b357c  Fixing some more pxi includes

comment:101 Changed 6 years ago by
Ok, I think I fixed the problems.
comment:102 Changed 6 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:103 Changed 6 years ago by
I think mine are passing. See Prec_Build.term on the project....
comment:104 Changed 6 years ago by
 Status changed from needs_review to positive_review
I updated my development branch and now everything is passing. It looks good to me! :)
comment:105 Changed 6 years ago by
 Keywords sd71 added
comment:106 Changed 6 years ago by
 Keywords days71 added; sd71 removed
 Reviewers changed from David Roe, Julian Rueth to David Roe, Julian Rueth, Aly Deines
comment:107 Changed 6 years ago by
 Reviewers changed from David Roe, Julian Rueth, Aly Deines to David Roe, Julian Rüth, Aly Deines
(I was told to be consistent about my last name…)
comment:108 Changed 6 years ago by
 Branch changed from public/14304 to 56b357c3f9b59853ec0ed7885a249b120ce58f95
 Resolution set to fixed
 Status changed from positive_review to 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.