Opened 7 years ago

Closed 4 years ago

#14304 closed enhancement (fixed)

New implementation of unramified p-adics using FLINT and templates

Reported by: roed Owned by: roed
Priority: major Milestone: sage-7.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) Commit: 56b357c3f9b59853ec0ed7885a249b120ce58f95
Dependencies: #20210 Stopgaps:

Description

Reimplement Qq using FLINT.

Change History (108)

comment:1 Changed 7 years ago by fredrik.johansson

  • Cc fredrik.johansson added

comment:2 Changed 7 years ago by jpflori

  • Cc jpflori added

comment:3 follow-up: Changed 7 years ago by roed

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.

Last edited 7 years ago by roed (previous) (diff)

comment:4 Changed 7 years ago by saraedum

  • Cc saraedum added

comment:5 in reply to: ↑ 3 Changed 6 years ago by jdemeyer

Replying to roed:

Once #12173 is positively reviewed

check!

comment:6 Changed 6 years ago by saraedum

comment:7 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:8 Changed 6 years ago by roed

  • Branch set to u/saraedum/Zq
  • Commit set to ed8db422d4707069b20cde96e8d7013112aa7591

Last 10 new commits:

ed8db42fixup
244b3a8conversion from finite fields
8f255a9fix some typos
65d3c03added matrix_mod_pn
ff52ce0removed FLINT skip
7b2ad9afixed a doctest
fd2ea9fadded flint rep
13a2d60added an _new_fmpz_poly method
52010f3switched to powcomputer_
02ffe57disabled tests for FLINT

comment:9 Changed 6 years ago by roed

  • 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 6 years ago by roed

  • Authors set to Julian Rueth, David Roe
  • Commit changed from ed8db422d4707069b20cde96e8d7013112aa7591 to e7afa8f9e65a29c5097031985248cae09f0483f4
  • Reviewers set to David Roe, Julian Rueth
  • Status changed from new to needs_review

New commits:

e7afa8fImproved some_elements()
ef18f02Adding some docstrings, fixing a few typos.

comment:11 Changed 6 years ago by saraedum

  • 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 6 years ago by saraedum

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 6 years ago by vbraun

Does not work... please run "make ptestlong"

comment:14 Changed 6 years ago by roed

  • Status changed from positive_review to needs_work

Will do.

comment:15 Changed 6 years ago by git

  • Commit changed from e7afa8f9e65a29c5097031985248cae09f0483f4 to 98a4529906267f0ece2f2a111fc2767ebdd8061d

Branch pushed to git repo; I updated commit sha1. New commits:

98a4529Fix doctest errors

comment:16 Changed 6 years ago by roed

  • Status changed from needs_work to needs_review

comment:17 follow-up: Changed 6 years ago by jdemeyer

  • 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 6 years ago by git

  • Commit changed from 98a4529906267f0ece2f2a111fc2767ebdd8061d to d0baddeae085ce6aabd34178aceb98a83e7673e5

Branch pushed to git repo; I updated commit sha1. New commits:

d0baddeFixing sig_on/sig_off pairs, adding BaseException to bare except:s

comment:19 in reply to: ↑ 17 ; follow-up: Changed 6 years ago by roed

  • 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 the raise (or use try/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 use except BaseException: or except 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 follow-up: Changed 6 years ago by vbraun

For the record, the Python docs say

exception BaseException

The base class for all built-in exceptions. It is not meant to be directly inherited by user-defined 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 no-op.

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 6 years ago by jdemeyer

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 6 years ago by jdemeyer

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 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:24 Changed 6 years ago by git

  • Commit changed from d0baddeae085ce6aabd34178aceb98a83e7673e5 to c647f4a1413547a47196f77acade2d7c45b46c28

Branch pushed to git repo; I updated commit sha1. New commits:

e101508Some sig_on, cython for loop fixes
c647f4aMore sig_on, cython for loop fixes

comment:25 in reply to: ↑ 20 ; follow-up: Changed 6 years ago by roed

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 no-op.

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 6 years ago by roed

  • 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 super-nested try blocks are ugly, but I don't know how to rewrite it safely without this nesting.

comment:27 in reply to: ↑ 25 ; follow-up: Changed 6 years ago by jpflori

Replying to roed:

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?

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 ; follow-up: Changed 6 years ago by roed

Replying to jpflori:

Replying to roed:

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?

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 6 years ago by jpflori

I'll have a look tomorrow (french time).

comment:30 in reply to: ↑ 28 ; follow-up: Changed 6 years ago by 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. The C-level 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()
Last edited 6 years ago by jdemeyer (previous) (diff)

comment:31 in reply to: ↑ 30 ; follow-up: Changed 6 years ago by saraedum

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 6 years ago by jdemeyer

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 6 years ago by git

  • Commit changed from c647f4a1413547a47196f77acade2d7c45b46c28 to 60975a4eed6b9bc665219709e337f801eb2b264d

Branch pushed to git repo; I updated commit sha1. New commits:

4f04d28Fix PowComputer_flint_unram.__cinit__ to not leak memory when interrupted.
60975a4Remove the 11-nested try blocks in the interest of code readability, sacrificing some memory-leak resistance

comment:34 Changed 6 years ago by roed

Here are two commits; the first has the 11-nested try blocks with "safe" memory handling, and the second removes them for code readability. I'm fine with either approach.

comment:35 follow-up: Changed 6 years ago by jdemeyer

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 ; follow-up: Changed 6 years ago by roed

Replying to jdemeyer:

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.

So the whole block of allocations should be wrapped in a single sig_on()/sig_off()?

comment:37 in reply to: ↑ 36 Changed 6 years ago by jdemeyer

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 follow-up: Changed 6 years ago by jpflori

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 6 years ago by jdemeyer

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.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:40 Changed 6 years ago by pbruin

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 follow-up: Changed 6 years ago by jdemeyer

For that to work properly, self.__allocated should have type volatile int.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 6 years ago by pbruin

Replying to jdemeyer:

For that to work properly, self.__allocated should have type volatile int.

OK, I can believe that. Does Cython support volatile?

comment:43 in reply to: ↑ 42 Changed 6 years ago by jdemeyer

Replying to pbruin:

Replying to jdemeyer:

For that to work properly, self.__allocated should have type volatile 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 6 years ago by vbraun

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 in-order 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 6 years ago by vbraun

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

  1. a compiler barrier (asm volatile("" ::: "memory") in gcc)
  2. move the fmpz_init into __init__
  3. RAII (separate classes for each fmpz_poly_t)

comment:46 Changed 6 years ago by git

  • Commit changed from 60975a4eed6b9bc665219709e337f801eb2b264d to cda1d750c9d7930b5997e036dd5a01a5398bbbbb

Branch pushed to git repo; I updated commit sha1. New commits:

cda1d75Wrap allocations in sig_on/sig_off

comment:47 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:48 Changed 5 years ago by pbruin

  • 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 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:50 Changed 5 years ago by saraedum

  • 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 5 years ago by saraedum

  • Keywords sd59 added
  • Status changed from needs_work to needs_review

comment:52 Changed 5 years ago by saraedum

  • 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:

f11fb44Merge branch 'develop' into ticket/14304
cce5076adapt to the new version of FLINT's padic_ctx_init
994be79Fixed a typo in linkage to FLINT for qadics
c4348c2fixed a doctest in padic factor

comment:53 Changed 5 years ago by git

  • Commit changed from c4348c282371ba0e8b53eb83f3ee0c10fd05d1e2 to 5b42f9bfb4db5cd99a749a17ba283feaa215abdc

Branch pushed to git repo; I updated commit sha1. New commits:

d4d1353Remove some trailing whitespace
13902d3Fix copy() for qadic coercion/conversion maps
3c1d9adAdd __richcmp__ for qadic element classes
5b42f9bFixed doctests.

comment:54 Changed 5 years ago by saraedum

I hope that this fixes all the bugs the patchbot detected.

comment:55 Changed 5 years ago by git

  • Commit changed from 5b42f9bfb4db5cd99a749a17ba283feaa215abdc to f33bd5bc704476a03c99f46156b046fa88b5e547

Branch pushed to git repo; I updated commit sha1. New commits:

2fcdde8Fix copy() for qadic coercion maps from capped absolute rings
f33bd5bMinor change to exception handling in MatrixSpace.matrix()

comment:56 Changed 5 years ago by git

  • Commit changed from f33bd5bc704476a03c99f46156b046fa88b5e547 to c7ffd25e860575755421c503a3189fbd928a0ab5

Branch pushed to git repo; I updated commit sha1. New commits:

c7ffd25Fix copy() for qadic conversion maps to capped absolute rings

comment:57 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:58 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

needs rebase

comment:59 Changed 4 years ago by git

  • Commit changed from c7ffd25e860575755421c503a3189fbd928a0ab5 to 9fecbdd438ecede712a97afe4edb2242eeb25cb0

Branch pushed to git repo; I updated commit sha1. New commits:

9fecbddMerge branch 'develop' into t/14304/ticket/14304

comment:60 Changed 4 years ago by git

  • Commit changed from 9fecbdd438ecede712a97afe4edb2242eeb25cb0 to 5ba4124ac05a5587a9c9dfc1be19f831d3a45874

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5ba4124Merge branch 'develop' into t/14304/ticket/14304

comment:61 Changed 4 years ago by git

  • Commit changed from 5ba4124ac05a5587a9c9dfc1be19f831d3a45874 to 5e17cdba912143101d8c32a5a8c1613366c9d49d

Branch pushed to git repo; I updated commit sha1. New commits:

e6bc498fixed some hashing doctests of q-adics
5e17cdbDo not rely on generic conversion to Rational when converting from finite fields to p-adics

comment:62 Changed 4 years ago by git

  • Commit changed from 5e17cdba912143101d8c32a5a8c1613366c9d49d to 2c3a71063fd13752a1f839a573191139fb1fc9ea

Branch pushed to git repo; I updated commit sha1. New commits:

2c3a710implement _cache_key for q-adics

comment:63 follow-up: Changed 4 years ago by jdemeyer

  • Dependencies #12555, #12173 deleted

Just a few technical comments (I don't plan to formally review this):

  1. You don't need to add include_dirs = [SAGE_INC + '/flint'] (just add flint/ to the path to the .h files)
  2. You don't need to add __richcmp__ calling _richcmp
  3. You don't need to add __hash__ which just raises TypeError: all Elements are now unhashable by default.
  4. For consistency with other .pxd files, could you move all declarations of FLINT types to sage/libs/flint/types.pxd?
  5. 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).

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:64 Changed 4 years ago by git

  • Commit changed from 2c3a71063fd13752a1f839a573191139fb1fc9ea to ab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a

Branch pushed to git repo; I updated commit sha1. New commits:

ab07bc4remove unnecessary include_dirs for FLINT from module_list.py

comment:65 Changed 4 years ago by git

  • Commit changed from ab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a to 70c780d5c1da3475135cba2db12903b59f1ade1b

Branch pushed to git repo; I updated commit sha1. New commits:

52425beMerge branch 'develop' into t/14304/ticket/14304
2a1f293remove unnecessary implementation of __hash__ and __richcmp__
6fbb0d7fix conversion from residue field to q-adic fields
70c780dMerge branch 'develop' into t/14304/ticket/14304

comment:66 Changed 4 years ago by git

  • Commit changed from 70c780d5c1da3475135cba2db12903b59f1ade1b to 0ab73f6de4d5ece9779841971aa33a16894f7c47

Branch pushed to git repo; I updated commit sha1. New commits:

0ab73f6Convert to p-adic residue field if there is a coercion

comment:67 Changed 4 years ago by jdemeyer

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. 
    1818#                  http://www.gnu.org/licenses/
    1919#*****************************************************************************
    2020
     21from sage.libs.flint.fmpz cimport *
     22from sage.libs.flint.fmpz_poly cimport *
     23from sage.libs.flint.ntl_interface cimport *
    2124from sage.structure.element cimport AlgebraElement, RingElement, ModuleElement, Element
    2225from sage.algebras.quatalg.quaternion_algebra_element cimport QuaternionAlgebraElement_abstract
    2326from sage.rings.rational cimport Rational

comment:68 Changed 4 years ago by jdemeyer

There are also some strange added imports in src/sage/rings/polynomial/...

comment:69 Changed 4 years ago by jdemeyer

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 
    22
    33from sage.libs.ntl.ntl_ZZ_decl cimport ZZ_c
    44from sage.libs.ntl.ntl_ZZX_decl cimport ZZX_c
     5from sage.libs.flint.fmpz cimport fmpz_t
     6from sage.libs.flint.fmpz_poly cimport fmpz_poly_t
    57
    68cdef extern from "flint/NTL-interface.h":
    79    void fmpz_poly_get_ZZX(ZZX_c output, fmpz_poly_t poly)

comment:70 Changed 4 years ago by saraedum

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.

Version 0, edited 4 years ago by saraedum (next)

comment:71 Changed 4 years ago by git

  • Commit changed from 0ab73f6de4d5ece9779841971aa33a16894f7c47 to 63d90c692635f9aa1ca2d87cf47f777dc5d6af03

Branch pushed to git repo; I updated commit sha1. New commits:

63d90c6remove duplicate imports

comment:72 Changed 4 years ago by git

  • Commit changed from 63d90c692635f9aa1ca2d87cf47f777dc5d6af03 to 94dda77df5900aa53ae2ffe51259c2b8331d18c5

Branch pushed to git repo; I updated commit sha1. New commits:

932d5ddRevert "remove unnecessary implementation of __hash__ and __richcmp__"
94dda77remove unnecessary implementation of __richcmp__

comment:73 Changed 4 years ago by git

  • Commit changed from 94dda77df5900aa53ae2ffe51259c2b8331d18c5 to d2ced66f3fbb8e9955b4d46baf02359b34b0f7f0

Branch pushed to git repo; I updated commit sha1. New commits:

d2ced66Fix conversion from finite rings to q-adic fields

comment:74 Changed 4 years ago by jdemeyer

  • Dependencies set to #19646

comment:75 Changed 4 years ago by jdemeyer

Conflicts with 6.10.beta6. If you fix this conflict, I can easily merge #19646.

comment:76 Changed 4 years ago by git

  • Commit changed from d2ced66f3fbb8e9955b4d46baf02359b34b0f7f0 to 1238235ff417f8c72553419f06d2cef7dd5d40a2

Branch pushed to git repo; I updated commit sha1. New commits:

1238235Merge branch 'develop' into t/14304/ticket/14304

comment:77 Changed 4 years ago by git

  • Commit changed from 1238235ff417f8c72553419f06d2cef7dd5d40a2 to 33df900f7f45683a6e0b23fb91bca43307c1a1fd

Branch pushed to git repo; I updated commit sha1. New commits:

3a68eb5Merge branch 'develop' of git://trac.sagemath.org/sage into t/14304/ticket/14304
33df900fix build errors in padics/pow_computer_flint

comment:78 Changed 4 years ago by git

  • Commit changed from 33df900f7f45683a6e0b23fb91bca43307c1a1fd to 73303b8a5aa8027b68d16a01883aefb7f821e609

Branch pushed to git repo; I updated commit sha1. New commits:

99fea64silence deprecation warning in padics factory
b522bebfixed incorrect doctests related to p-adic hashing
73303b8fixed identity morphism for unramified p-adics

comment:79 Changed 4 years ago by saraedum

  • Status changed from needs_work to needs_review

All doctests in rings/ now pass (have not checked with --long.)

comment:80 Changed 4 years ago by git

  • Commit changed from 73303b8a5aa8027b68d16a01883aefb7f821e609 to 8291b03d4803982922e53573948681ed7d64e9ec

Branch pushed to git repo; I updated commit sha1. New commits:

89a56a0implement residue() for q-adic elements
12c7cc3Fix p-adic doctests
8291b03implement conversion from polynomials to q-adics

comment:81 Changed 4 years ago by jdemeyer

  • Dependencies #19646 deleted
  • Milestone changed from sage-6.4 to sage-7.0

comment:82 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

See 63

comment:83 follow-up: Changed 4 years ago by jdemeyer

Avoid cdefs.pxi, just cimport whatever you need.

comment:84 Changed 4 years ago by git

  • Commit changed from 8291b03d4803982922e53573948681ed7d64e9ec to 4b33f86e947f3ae1b3b35f311bb18f38bb62217c

Branch pushed to git repo; I updated commit sha1. New commits:

e376e21Merge branch 'develop' into t/14304/ticket/14304
860366dremove except: for C function call in pow_computer
4b33f86fix build error

comment:85 in reply to: ↑ 63 Changed 4 years ago by saraedum

Replying to jdemeyer:

  1. You don't need to add include_dirs = [SAGE_INC + '/flint'] (just add flint/ to the path to the .h files)

Fixed in ab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a.

  1. You don't need to add __richcmp__ calling _richcmp

Fixed in 2a1f29345297da21ffe14c47e39faa1ac76eda5a.

  1. You don't need to add __hash__ which just raises TypeError: all Elements are now unhashable by default.

I actually do need this implementation to override the base class. Adressed in 2a1f29345297da21ffe14c47e39faa1ac76eda5a.

  1. For consistency with other .pxd files, could you move all declarations of FLINT types to sage/libs/flint/types.pxd?

I believe that this has happened by now.

  1. 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).

Both fixed in 860366d0bf36e8684c97f5bd5d3e336e79f5795d.


New commits:

e376e21Merge branch 'develop' into t/14304/ticket/14304
860366dremove except: for C function call in pow_computer
4b33f86fix build error

comment:86 in reply to: ↑ 83 Changed 4 years ago by saraedum

Replying to jdemeyer:

Avoid cdefs.pxi, just cimport whatever you need.

Fixed.

comment:87 Changed 4 years ago by git

  • Commit changed from 4b33f86e947f3ae1b3b35f311bb18f38bb62217c to e8c1e41641ced2f346c906c984fb978aa3efc1ce

Branch pushed to git repo; I updated commit sha1. New commits:

e8c1e41remove cdefs.pxi imports in qadics

comment:88 Changed 4 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:89 Changed 4 years ago by chapoton

  • Branch changed from u/saraedum/ticket/14304 to public/14304
  • Commit changed from e8c1e41641ced2f346c906c984fb978aa3efc1ce to 87ee786dbea263b0d0e587f5d27fd5f35aa84e62

I just corrected a raise statement. There seems to be many failing doctests.


New commits:

052d171Merge branch 'u/saraedum/ticket/14304' into 7.1
87ee786trac #14304 correct one raise into py3 format, plus some doc correction

comment:90 Changed 4 years ago by chapoton

  • Milestone changed from sage-7.0 to sage-7.2

comment:91 Changed 4 years ago by git

  • Commit changed from 87ee786dbea263b0d0e587f5d27fd5f35aa84e62 to 26c8669ca7cffe6b2a271a547e16245a59ede61f

Branch pushed to git repo; I updated commit sha1. New commits:

c3b41a4Merge branch 'develop' into t/14303/ticket/14304
2141bd9Fix bug that would be triggered for ramified extensions
9c4aecbImprove docstring to use .. WARNING
354bc5eBreak up (some of) the long lines
b422cfcMerge branch 'public/14304' of git://trac.sagemath.org/sage into t/14304/ticket/14304
26c8669Fix typo in pow computer

comment:92 Changed 4 years ago by aly.deines

  • Status changed from needs_review to needs_work

On line 494 in sage/local/lib/python2.7/site-packages/sage/rings/padics/factory.py, you need to break the line using parenthesis.

aly@aly-laptop:~/Sage/sage$ ./sage -t src/sage
Traceback (most recent call last):
  File "/home/aly/Sage/sage/src/bin/sage-runtests", line 80, in <module>
    from sage.doctest.control import DocTestController
  File "/home/aly/Sage/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 30, in <module>
    from sources import FileDocTestSource, DictAsObject
  File "/home/aly/Sage/sage/local/lib/python2.7/site-packages/sage/doctest/sources.py", line 27, in <module>
    from parsing import SageDocTestParser
  File "/home/aly/Sage/sage/local/lib/python2.7/site-packages/sage/doctest/parsing.py", line 49, in <module>
    from sage.all import RealIntervalField
  File "/home/aly/Sage/sage/local/lib/python2.7/site-packages/sage/all.py", line 100, in <module>
    from sage.rings.all      import *
  File "/home/aly/Sage/sage/local/lib/python2.7/site-packages/sage/rings/all.py", line 62, in <module>
    from padics.all import *
  File "/home/aly/Sage/sage/local/lib/python2.7/site-packages/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/site-packages/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 4 years ago by git

  • Commit changed from 26c8669ca7cffe6b2a271a547e16245a59ede61f to baca1928045614982f170e4efc5f56585c223352

Branch pushed to git repo; I updated commit sha1. New commits:

baca192Add parentheses for line wraps in factory.pyx

comment:94 Changed 4 years ago by roed

  • Status changed from needs_work to needs_review

Fixed

comment:95 Changed 4 years ago by jdemeyer

  • Dependencies set to #20210
  • Status changed from needs_review to needs_work

Needs to be rebased on top of #20210.

comment:96 Changed 4 years ago by git

  • Commit changed from baca1928045614982f170e4efc5f56585c223352 to c22ae81406ebc5b496227a2d2547a89363e4fbfe

Branch pushed to git repo; I updated commit sha1. New commits:

03458eaUpgrade cysignals package
dce67fcMove memory functions to cysignals
4bb8337Rename sage_malloc -> sig_malloc and friends
c22ae81Merge commit '4bb8337295ed82c93d1a9c9f7173a4c3' into t/14304/ticket/14304

comment:97 Changed 4 years ago by git

  • Commit changed from c22ae81406ebc5b496227a2d2547a89363e4fbfe to a14ee5d2d39b3ddd4e8bcbd76922af8bf3aeb213

Branch pushed to git repo; I updated commit sha1. New commits:

a14ee5dFixing imports and name changes due to switch to cysignals

comment:98 Changed 4 years ago by roed

Okay, I think I rebased it, but there are a ton of doctest errors that have nothing to do with p-adics. 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 4 years ago by roed

  • Status changed from needs_work to needs_review

comment:100 Changed 4 years ago by git

  • Commit changed from a14ee5d2d39b3ddd4e8bcbd76922af8bf3aeb213 to 56b357c3f9b59853ec0ed7885a249b120ce58f95

Branch pushed to git repo; I updated commit sha1. New commits:

56b357cFixing some more pxi includes

comment:101 Changed 4 years ago by roed

Ok, I think I fixed the problems.

comment:102 Changed 4 years ago by aly.deines

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 4 years ago by roed

I think mine are passing. See Prec_Build.term on the project....

comment:104 Changed 4 years ago by aly.deines

  • 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 4 years ago by aly.deines

  • Keywords sd71 added

comment:106 Changed 4 years ago by aly.deines

  • Keywords days71 added; sd71 removed
  • Reviewers changed from David Roe, Julian Rueth to David Roe, Julian Rueth, Aly Deines

comment:107 Changed 4 years ago by saraedum

  • Authors changed from Julian Rueth, David Roe to Julian Rüth, David Roe
  • 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 4 years ago by vbraun

  • Branch changed from public/14304 to 56b357c3f9b59853ec0ed7885a249b120ce58f95
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.