Opened 10 years ago

Closed 7 years ago

#14304 closed enhancement (fixed)

New implementation of unramified p-adics using FLINT and templates

Reported by: David Roe Owned by: David Roe
Priority: major Milestone: sage-7.2
Component: padics Keywords: sd59, days71
Cc: Fredrik Johansson, Jean-Pierre 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:

Status badges

Description

Reimplement Qq using FLINT.

Change History (108)

comment:1 Changed 10 years ago by Fredrik Johansson

Cc: Fredrik Johansson added

comment:2 Changed 10 years ago by Jean-Pierre Flori

Cc: Jean-Pierre Flori added

comment:3 Changed 10 years ago by David Roe

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 10 years ago by David Roe (previous) (diff)

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

Cc: Julian Rüth added

comment:5 in reply to:  3 Changed 10 years ago by Jeroen Demeyer

Replying to roed:

Once #12173 is positively reviewed

check!

comment:6 Changed 9 years ago by Julian Rüth

comment:7 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:8 Changed 9 years ago by David Roe

Branch: u/saraedum/Zq
Commit: 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 9 years ago by David Roe

Branch: u/saraedum/Zqu/roed/ticket/14304
Created: Mar 19, 2013, 3:14:00 AMMar 19, 2013, 3:14:00 AM
Modified: Jan 5, 2014, 12:38:49 AMJan 5, 2014, 12:38:49 AM

comment:10 Changed 9 years ago by David Roe

Authors: Julian Rueth, David Roe
Commit: ed8db422d4707069b20cde96e8d7013112aa7591e7afa8f9e65a29c5097031985248cae09f0483f4
Reviewers: David Roe, Julian Rueth
Status: newneeds_review

New commits:

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

comment:11 Changed 9 years ago by Julian Rüth

Modified: Jan 5, 2014, 8:49:26 PMJan 5, 2014, 8:49:26 PM
Status: needs_reviewpositive_review

comment:12 Changed 9 years ago by Julian Rüth

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 9 years ago by Volker Braun

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

comment:14 Changed 9 years ago by David Roe

Status: positive_reviewneeds_work

Will do.

comment:15 Changed 9 years ago by git

Commit: e7afa8f9e65a29c5097031985248cae09f0483f498a4529906267f0ece2f2a111fc2767ebdd8061d

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

98a4529Fix doctest errors

comment:16 Changed 9 years ago by David Roe

Status: needs_workneeds_review

comment:17 Changed 9 years ago by Jeroen Demeyer

Status: needs_reviewneeds_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 git

Commit: 98a4529906267f0ece2f2a111fc2767ebdd8061dd0baddeae085ce6aabd34178aceb98a83e7673e5

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 ; Changed 9 years ago by David Roe

Status: needs_workneeds_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 Changed 9 years ago by Volker Braun

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 9 years ago by Jeroen Demeyer

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 Jeroen Demeyer

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 Jeroen Demeyer

Status: needs_reviewneeds_work

comment:24 Changed 9 years ago by git

Commit: d0baddeae085ce6aabd34178aceb98a83e7673e5c647f4a1413547a47196f77acade2d7c45b46c28

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 ; Changed 9 years ago by David Roe

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 9 years ago by David Roe

Status: needs_workneeds_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 ; Changed 9 years ago by Jean-Pierre Flori

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 ; Changed 9 years ago by David Roe

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 9 years ago by Jean-Pierre Flori

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

comment:30 in reply to:  28 ; Changed 9 years ago by Jeroen Demeyer

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 9 years ago by Jeroen Demeyer (previous) (diff)

comment:31 in reply to:  30 ; Changed 9 years ago by Julian Rüth

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 9 years ago by Jeroen Demeyer

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 git

Commit: c647f4a1413547a47196f77acade2d7c45b46c2860975a4eed6b9bc665219709e337f801eb2b264d

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 9 years ago by David Roe

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 Changed 9 years ago by Jeroen Demeyer

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 ; Changed 9 years ago by David Roe

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 9 years ago by Jeroen Demeyer

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 Changed 9 years ago by Jean-Pierre Flori

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 9 years ago by Jeroen Demeyer

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 9 years ago by Jeroen Demeyer (previous) (diff)

comment:40 Changed 9 years ago by Peter Bruin

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 Changed 9 years ago by Jeroen Demeyer

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

comment:42 in reply to:  41 ; Changed 9 years ago by Peter Bruin

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 9 years ago by Jeroen Demeyer

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 9 years ago by Volker Braun

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 9 years ago by Volker Braun

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

Commit: 60975a4eed6b9bc665219709e337f801eb2b264dcda1d750c9d7930b5997e036dd5a01a5398bbbbb

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

cda1d75Wrap allocations in sig_on/sig_off

comment:47 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:48 Changed 9 years ago by Peter Bruin

Status: needs_reviewneeds_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 For batch modifications

Milestone: sage-6.2sage-6.3

comment:50 Changed 8 years ago by Julian Rüth

Branch: u/roed/ticket/14304u/saraedum/ticket/14304
Modified: May 6, 2014, 3:20:58 PMMay 6, 2014, 3:20:58 PM

comment:51 Changed 8 years ago by Julian Rüth

Keywords: sd59 added
Status: needs_workneeds_review

comment:52 Changed 8 years ago by Julian Rüth

Commit: cda1d750c9d7930b5997e036dd5a01a5398bbbbbc4348c282371ba0e8b53eb83f3ee0c10fd05d1e2

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

Commit: c4348c282371ba0e8b53eb83f3ee0c10fd05d1e25b42f9bfb4db5cd99a749a17ba283feaa215abdc

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 8 years ago by Julian Rüth

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

comment:55 Changed 8 years ago by git

Commit: 5b42f9bfb4db5cd99a749a17ba283feaa215abdcf33bd5bc704476a03c99f46156b046fa88b5e547

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

Commit: f33bd5bc704476a03c99f46156b046fa88b5e547c7ffd25e860575755421c503a3189fbd928a0ab5

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

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

comment:57 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:58 Changed 8 years ago by Frédéric Chapoton

Status: needs_reviewneeds_work

needs rebase

comment:59 Changed 7 years ago by git

Commit: c7ffd25e860575755421c503a3189fbd928a0ab59fecbdd438ecede712a97afe4edb2242eeb25cb0

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

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

comment:60 Changed 7 years ago by git

Commit: 9fecbdd438ecede712a97afe4edb2242eeb25cb05ba4124ac05a5587a9c9dfc1be19f831d3a45874

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

Commit: 5ba4124ac05a5587a9c9dfc1be19f831d3a458745e17cdba912143101d8c32a5a8c1613366c9d49d

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

Commit: 5e17cdba912143101d8c32a5a8c1613366c9d49d2c3a71063fd13752a1f839a573191139fb1fc9ea

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

2c3a710implement _cache_key for q-adics

comment:63 Changed 7 years ago by Jeroen Demeyer

Dependencies: #12555, #12173

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 7 years ago by Jeroen Demeyer (previous) (diff)

comment:64 Changed 7 years ago by git

Commit: 2c3a71063fd13752a1f839a573191139fb1fc9eaab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a

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

ab07bc4remove unnecessary include_dirs for FLINT from module_list.py

comment:65 Changed 7 years ago by git

Commit: ab07bc41a76f58c6063c5e7a9ab8b4add7bc4f6a70c780d5c1da3475135cba2db12903b59f1ade1b

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

Commit: 70c780d5c1da3475135cba2db12903b59f1ade1b0ab73f6de4d5ece9779841971aa33a16894f7c47

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 7 years ago by Jeroen Demeyer

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 7 years ago by Jeroen Demeyer

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

comment:69 Changed 7 years ago by Jeroen Demeyer

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 7 years ago by Julian Rüth

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.

Last edited 7 years ago by Julian Rüth (previous) (diff)

comment:71 Changed 7 years ago by git

Commit: 0ab73f6de4d5ece9779841971aa33a16894f7c4763d90c692635f9aa1ca2d87cf47f777dc5d6af03

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

63d90c6remove duplicate imports

comment:72 Changed 7 years ago by git

Commit: 63d90c692635f9aa1ca2d87cf47f777dc5d6af0394dda77df5900aa53ae2ffe51259c2b8331d18c5

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

Commit: 94dda77df5900aa53ae2ffe51259c2b8331d18c5d2ced66f3fbb8e9955b4d46baf02359b34b0f7f0

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

d2ced66Fix conversion from finite rings to q-adic fields

comment:74 Changed 7 years ago by Jeroen Demeyer

Dependencies: #19646

comment:75 Changed 7 years ago by Jeroen Demeyer

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

comment:76 Changed 7 years ago by git

Commit: d2ced66f3fbb8e9955b4d46baf02359b34b0f7f01238235ff417f8c72553419f06d2cef7dd5d40a2

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

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

comment:77 Changed 7 years ago by git

Commit: 1238235ff417f8c72553419f06d2cef7dd5d40a233df900f7f45683a6e0b23fb91bca43307c1a1fd

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

Commit: 33df900f7f45683a6e0b23fb91bca43307c1a1fd73303b8a5aa8027b68d16a01883aefb7f821e609

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 7 years ago by Julian Rüth

Status: needs_workneeds_review

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

comment:80 Changed 7 years ago by git

Commit: 73303b8a5aa8027b68d16a01883aefb7f821e6098291b03d4803982922e53573948681ed7d64e9ec

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 7 years ago by Jeroen Demeyer

Dependencies: #19646
Milestone: sage-6.4sage-7.0

comment:82 Changed 7 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

See 63

comment:83 Changed 7 years ago by Jeroen Demeyer

Avoid cdefs.pxi, just cimport whatever you need.

comment:84 Changed 7 years ago by git

Commit: 8291b03d4803982922e53573948681ed7d64e9ec4b33f86e947f3ae1b3b35f311bb18f38bb62217c

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 7 years ago by Julian Rüth

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 7 years ago by Julian Rüth

Replying to jdemeyer:

Avoid cdefs.pxi, just cimport whatever you need.

Fixed.

comment:87 Changed 7 years ago by git

Commit: 4b33f86e947f3ae1b3b35f311bb18f38bb62217ce8c1e41641ced2f346c906c984fb978aa3efc1ce

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

e8c1e41remove cdefs.pxi imports in qadics

comment:88 Changed 7 years ago by Julian Rüth

Status: needs_workneeds_review

comment:89 Changed 7 years ago by Frédéric Chapoton

Branch: u/saraedum/ticket/14304public/14304
Commit: e8c1e41641ced2f346c906c984fb978aa3efc1ce87ee786dbea263b0d0e587f5d27fd5f35aa84e62

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 7 years ago by Frédéric Chapoton

Milestone: sage-7.0sage-7.2

comment:91 Changed 7 years ago by git

Commit: 87ee786dbea263b0d0e587f5d27fd5f35aa84e6226c8669ca7cffe6b2a271a547e16245a59ede61f

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 7 years ago by Alyson Deines

Status: needs_reviewneeds_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 7 years ago by git

Commit: 26c8669ca7cffe6b2a271a547e16245a59ede61fbaca1928045614982f170e4efc5f56585c223352

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

baca192Add parentheses for line wraps in factory.pyx

comment:94 Changed 7 years ago by David Roe

Status: needs_workneeds_review

Fixed

comment:95 Changed 7 years ago by Jeroen Demeyer

Dependencies: #20210
Status: needs_reviewneeds_work

Needs to be rebased on top of #20210.

comment:96 Changed 7 years ago by git

Commit: baca1928045614982f170e4efc5f56585c223352c22ae81406ebc5b496227a2d2547a89363e4fbfe

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

Commit: c22ae81406ebc5b496227a2d2547a89363e4fbfea14ee5d2d39b3ddd4e8bcbd76922af8bf3aeb213

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

a14ee5dFixing imports and name changes due to switch to cysignals

comment:98 Changed 7 years ago by David Roe

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 7 years ago by David Roe

Status: needs_workneeds_review

comment:100 Changed 7 years ago by git

Commit: a14ee5d2d39b3ddd4e8bcbd76922af8bf3aeb21356b357c3f9b59853ec0ed7885a249b120ce58f95

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

56b357cFixing some more pxi includes

comment:101 Changed 7 years ago by David Roe

Ok, I think I fixed the problems.

comment:102 Changed 7 years ago by Alyson 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 7 years ago by David Roe

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

comment:104 Changed 7 years ago by Alyson Deines

Status: needs_reviewpositive_review

I updated my development branch and now everything is passing. It looks good to me! :)

comment:105 Changed 7 years ago by Alyson Deines

Keywords: sd71 added

comment:106 Changed 7 years ago by Alyson Deines

Keywords: days71 added; sd71 removed
Reviewers: David Roe, Julian RuethDavid Roe, Julian Rueth, Aly Deines

comment:107 Changed 7 years ago by Julian Rüth

Authors: Julian Rueth, David RoeJulian Rüth, David Roe
Reviewers: David Roe, Julian Rueth, Aly DeinesDavid Roe, Julian Rüth, Aly Deines

(I was told to be consistent about my last name…)

comment:108 Changed 7 years ago by Volker Braun

Branch: public/1430456b357c3f9b59853ec0ed7885a249b120ce58f95
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.