Opened 7 years ago

Closed 7 years ago

#20226 closed enhancement (fixed)

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

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-7.3
Component: interfaces Keywords: days77, days74
Cc: Luca De Feo, Jean-Pierre Flori Merged in:
Authors: Jeroen Demeyer Reviewers: Luca De Feo, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: b550855 (Commits, GitHub, GitLab) Commit: b55085547315715f292fd08ce74ad5a437abc6a5
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

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

Change History (39)

comment:1 Changed 7 years ago by Jeroen Demeyer

Branch: u/jdemeyer/ticket/20226

comment:2 Changed 7 years ago by Jeroen Demeyer

Commit: e871a64ead4b259543612e2c247a9aaf0cf1bf12
Description: modified (diff)
Summary: Implement conversion PARI <-> Python int/long without mpzImplement conversion PARI <-> Python int/long without GMP/MPIR

New commits:

03458eaUpgrade cysignals package
dce67fcMove memory functions to cysignals
4bb8337Rename sage_malloc -> sig_malloc and friends
5ab73c1Get rid of factorint_withproof_sage in PARI interface
c0ed97aStop using deprecated PARI factoring features
edc5ce2Merge branch 't/20205/get_rid_of_factorint_withproof_sage_in_pari_interface' into HEAD
5fb408dReplace pari_catch_sig_on by sig_on
d5c934cDeprecate PARI nth_prime and prime_list
d7d2d7dRemove redundant functions from pari_instance.pyx
e871a64Implement conversion PARI <-> Python int/long without mpz

comment:3 Changed 7 years ago by Jeroen Demeyer

Dependencies: #20210, #20205, #20213, #20216, #20217#20210, #20205, #20213, #20216, #20217, #20231

comment:4 Changed 7 years ago by Jeroen Demeyer

Cc: Luca De Feo added
Status: newneeds_review

comment:5 Changed 7 years ago by git

Commit: e871a64ead4b259543612e2c247a9aaf0cf1bf12c5f9892d7db87ba858c4cc6131e379796cbe7769

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

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

comment:6 Changed 7 years ago by Jeroen Demeyer

Dependencies: #20210, #20205, #20213, #20216, #20217, #20231
Milestone: sage-7.1sage-7.2

comment:7 Changed 7 years ago by Luca De Feo

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.

comment:8 Changed 7 years ago by git

Commit: c5f9892d7db87ba858c4cc6131e379796cbe7769fa2081de3e7b906b60909ed6ea7988ee496d3a59

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

fa2081dAdd some documentation for PARI conversion

comment:9 Changed 7 years ago by Luca De Feo

Reviewers: Luca De Feo
Status: needs_reviewpositive_review

Thanks

comment:10 Changed 7 years ago by Volker Braun

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
**********************************************************************

comment:11 Changed 7 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

comment:12 Changed 7 years ago by Jeroen Demeyer

That's a PARI bug...

comment:13 Changed 7 years ago by git

Commit: fa2081de3e7b906b60909ed6ea7988ee496d3a5947df94cc7c5181f99717d164b1929c516d039ec1

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

47df94cFix conversion of pari(LONG_MIN) to Python int

comment:14 Changed 7 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:15 Changed 7 years ago by Jeroen Demeyer

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

comment:16 Changed 7 years ago by Jeroen Demeyer

Keywords: days77 added

comment:17 Changed 7 years ago by Vincent Delecroix

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.

comment:18 in reply to:  17 ; Changed 7 years ago by Jeroen Demeyer

Replying to vdelecroix:

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?

comment:19 Changed 7 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

comment:20 in reply to:  18 ; Changed 7 years ago by Vincent Delecroix

Replying to jdemeyer:

Replying to vdelecroix:

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.

comment:21 in reply to:  20 Changed 7 years ago by Jeroen Demeyer

Replying to vdelecroix:

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

comment:22 Changed 7 years ago by Jeroen Demeyer

Reviewers: Luca De FeoLuca De Feo, Vincent Delecroix

comment:23 Changed 7 years ago by git

Commit: 47df94cc7c5181f99717d164b1929c516d039ec18668603e309601df7321762e499c1a2063c35fa3

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

8668603Add doctests for corner cases

comment:24 Changed 7 years ago by Jeroen Demeyer

Status: needs_workneeds_review

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.

comment:25 Changed 7 years ago by Erik Bray

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?

comment:26 Changed 7 years ago by Jeroen Demeyer

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

comment:27 Changed 7 years ago by Erik Bray

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

comment:28 Changed 7 years ago by Jean-Pierre Flori

Cc: Jean-Pierre Flori added

comment:29 Changed 7 years ago by git

Commit: 8668603e309601df7321762e499c1a2063c35fa3b55085547315715f292fd08ce74ad5a437abc6a5

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

c2305c0Merge tag '7.2' into t/20226/ticket/20226
b550855Fix compiler flag for GCC 4.8 issue

comment:30 Changed 7 years ago by Jeroen Demeyer

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

comment:31 Changed 7 years ago by Erik Bray

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

comment:32 in reply to:  31 Changed 7 years ago by Jeroen Demeyer

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

comment:33 Changed 7 years ago by Vincent Delecroix

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.

comment:34 in reply to:  33 Changed 7 years ago by Jeroen Demeyer

Replying to vdelecroix:

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.

comment:35 Changed 7 years ago by Vincent Delecroix

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;}                                           
Last edited 7 years ago by Vincent Delecroix (previous) (diff)

comment:36 Changed 7 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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

comment:37 Changed 7 years ago by Jeroen Demeyer

Status: needs_workneeds_review

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

comment:38 Changed 7 years ago by Vincent Delecroix

Keywords: days74 added
Milestone: sage-7.2sage-7.3
Status: needs_reviewpositive_review

comment:39 Changed 7 years ago by Volker Braun

Branch: u/jdemeyer/ticket/20226b55085547315715f292fd08ce74ad5a437abc6a5
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.