Implement conversion PARI <-> Python int/long without GMP/MPIR

[jdemeyer]

Convert PARI integers to/from Python integers without taking a detour via mpz_t.

CC: @defeo @jpflori

Component: interfaces

Keywords: days77, days74

Author: Jeroen Demeyer

Branch/Commit: b550855

Reviewer: Luca De Feo, Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/20226

[jdemeyer]

Branch: u/jdemeyer/ticket/20226

[jdemeyer]

Commit: e871a64

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1 +1 @@
-Convert PARI integers to/from Python integers without relying on the GMP/MPIR library.
+Convert PARI integers to/from Python integers without taking a detour via `mpz_t`.

[jdemeyer]

New commits:

03458ea	Upgrade cysignals package
dce67fc	Move memory functions to cysignals
4bb8337	Rename sage_malloc -> sig_malloc and friends
5ab73c1	Get rid of factorint_withproof_sage in PARI interface
c0ed97a	Stop using deprecated PARI factoring features
edc5ce2	Merge branch 't/20205/get_rid_of_factorint_withproof_sage_in_pari_interface' into HEAD
5fb408d	Replace pari_catch_sig_on by sig_on
d5c934c	Deprecate PARI nth_prime and prime_list
d7d2d7d	Remove redundant functions from pari_instance.pyx
e871a64	Implement conversion PARI <-> Python int/long without mpz

[jdemeyer]

Changed dependencies from #20210, #20205, #20213, #20216, #20217 to #20210, #20205, #20213, #20216, #20217, #20231

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from e871a64 to c5f9892

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

c5f9892

Implement conversion PARI <-> Python int/long without mpz

[jdemeyer]

Changed dependencies from #20210, #20205, #20213, #20216, #20217, #20231 to none

[defeo]

comment:7

It runs ok and all tests pass.

Minor drawbacks:

• it would be useful to have links to the Python and Pari longint specs.
• I am personally against from ... import *, but won't block the ticket if you want to keep it.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

fa2081d

Add some documentation for PARI conversion

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from c5f9892 to fa2081d

[defeo]

Reviewer: Luca De Feo

[defeo]

comment:9

Thanks

[vbraun]

comment:10

sage -t --long src/sage/libs/pari/gen.pyx
**********************************************************************
File "src/sage/libs/pari/gen.pyx", line 1343, in sage.libs.pari.gen.gen.__int__
Failed example:
    int(pari(-2^31))
Expected:
    -2147483648
Got:
    -2147483648L
**********************************************************************

[jdemeyer]

comment:12

That's a PARI bug...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from fa2081d to 47df94c

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

47df94c

Fix conversion of pari(LONG_MIN) to Python int

[jdemeyer]

comment:15

It's not a PARI bug, it's a PARI feature :-)

[jdemeyer]

Changed keywords from none to days77

[videlec]

comment:17

Would it be possible to test the subtle LONG_MIN case in gen_to_integer? Maybe along

sage: a = gen_to_integer(pari("2^63-1")); a; type(a)
9223372036854775807
<type 'int'>
sage: a = gen_to_integer(pari("2^63")); a; type(a)
9223372036854775808L
<type 'long'>
sage: a = gen_to_integer(pari("-2^63")); a; type(a)
-9223372036854775808
<type 'int'>
sage: a = gen_to_integer(pari("-2^63-1")); a; type(a)
-9223372036854775809L
<type 'long'>

What do you think of moving gtoi into PARI (not necessarily for this ticket)? This function is only about PARI's internal.

[jdemeyer]

comment:18

Replying to @videlec:

What do you think of moving gtoi into PARI (not necessarily for this ticket)? This function is only about PARI's internal.

Are you still in Bordeaux physically? If so, maybe you could talk to PARI upstream about this?

[videlec]

comment:20

Replying to @jdemeyer:

Replying to @videlec:

What do you think of moving gtoi into PARI (not necessarily for this ticket)? This function is only about PARI's internal.

Are you still in Bordeaux physically? If so, maybe you could talk to PARI upstream about this?

Sadly not (I am until september in Santiago de Chile). However, I can submit a patch if not already done.

[jdemeyer]

comment:21

Replying to @videlec:

Sadly not (I am until september in Santiago de Chile).

Doesn't sound so sad :-)

However, I can submit a patch if not already done.

For the moment, I lack the mental energy to try to push yet another patch to upstream PARI...

[jdemeyer]

Changed reviewer from Luca De Feo to Luca De Feo, Vincent Delecroix

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

8668603

Add doctests for corner cases

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 47df94c to 8668603

[jdemeyer]

comment:24

I added a test which checks that the conversion of integers via PARI is the same as the Python conversion str -> int. By checking that the result is the same as Python, we don't need to special-case 32-bit vs. 64-bit.

[embray]

comment:25

So, what specifically was the issue with this that it required blacklisting GCC 4.8? Is there a simple way to reproduce the issue, or at least a specific doctest that exhibits it that I could run?

[jdemeyer]

comment:26

It was some doctest in src/sage/libs/pari/convert.pyx. Let me know if you're unable to reproduce it.

[embray]

comment:27

Okay, I'll give it a look. I appreciate the need for the blacklisting for now just to get moving on things. But it will be good to fix if I can. I have set SAGE_INSTALL_GCC=no on my system, but that will mean I'll ultimately have a broken Sage if we can't resolve this :(

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

c2305c0	Merge tag '7.2' into t/20226/ticket/20226
b550855	Fix compiler flag for GCC 4.8 issue

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 8668603 to b550855

[jdemeyer]

comment:30

I'm now using the flag -fno-tree-copyrename which seems to fix all instances of the GCC 4.8 problem.

[embray]

comment:31

Interesting--not sure why that would do it but okay. But if it's not a problem on newer GCC versions maybe whatever the problem is is since fixed.

This might have something to do with it: https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01460.html

[jdemeyer]

comment:32

Replying to @embray:

Interesting--not sure why that would do it but okay. But if it's not a problem on newer GCC versions maybe whatever the problem is is since fixed.

The problem is indeed fixed in GCC-4.9, see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56982

[videlec]

comment:33

Could you use the macro Py_SIZE instead of calling py_long_object->ob_size? In particular, it will simplify the switch to Python3. Sadly, Cython does not accept the macro expansion Py_SIZE(my_object) = whatever and the C-API does not document an alternative way of doing things.

[jdemeyer]

comment:34

Replying to @videlec:

In particular, it will simplify the switch to Python3.

I don't understand why: ob_size still exists in Python 3.

Especially given that we cannot assign to Py_SIZE(), I don't really see the point.

[videlec]

comment:35

In Python 3 (<PyLongObject*>x).ob_size just doesn't compile

.../cython_test.c: In function ‘__pyx_pf_11cython_test_f’:
.../cython_test.c:593:61: error: ‘PyLongObject’ has no member named ‘ob_size’
   __pyx_t_1 = PyInt_FromSsize_t(((PyLongObject *)__pyx_v_s)->ob_size); if (unlikely(!__pyx_t_1)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 8; __pyx_clineno = __LINE__; goto __pyx_L1_error;}                                           

[jdemeyer]

comment:36

I see. Python 3 adds an extra level of indirection ob_base->ob_size.

[jdemeyer]

comment:37

On the other hand, to avoid getting stuck, I would prefer to defer the Python 3 porting to a new ticket.

[videlec]

Changed keywords from days77 to days77, days74

[vbraun]

Changed branch from u/jdemeyer/ticket/20226 to b550855