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:  sage7.3 
Component:  interfaces  Keywords:  days77, days74 
Cc:  Luca De Feo, JeanPierre 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: 
Description (last modified by )
Convert PARI integers to/from Python integers without taking a detour via mpz_t
.
Change History (39)
comment:1 Changed 7 years ago by
Branch:  → u/jdemeyer/ticket/20226 

comment:2 Changed 7 years ago by
Commit:  → e871a64ead4b259543612e2c247a9aaf0cf1bf12 

Description:  modified (diff) 
Summary:  Implement conversion PARI <> Python int/long without mpz → Implement conversion PARI <> Python int/long without GMP/MPIR 
comment:3 Changed 7 years ago by
Dependencies:  #20210, #20205, #20213, #20216, #20217 → #20210, #20205, #20213, #20216, #20217, #20231 

comment:4 Changed 7 years ago by
Cc:  Luca De Feo added 

Status:  new → needs_review 
comment:5 Changed 7 years ago by
Commit:  e871a64ead4b259543612e2c247a9aaf0cf1bf12 → c5f9892d7db87ba858c4cc6131e379796cbe7769 

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

comment:6 Changed 7 years ago by
Dependencies:  #20210, #20205, #20213, #20216, #20217, #20231 

Milestone:  sage7.1 → sage7.2 
comment:7 Changed 7 years ago by
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
Commit:  c5f9892d7db87ba858c4cc6131e379796cbe7769 → fa2081de3e7b906b60909ed6ea7988ee496d3a59 

Branch pushed to git repo; I updated commit sha1. New commits:
fa2081d  Add some documentation for PARI conversion

comment:9 Changed 7 years ago by
Reviewers:  → Luca De Feo 

Status:  needs_review → positive_review 
Thanks
comment:10 Changed 7 years ago by
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
Status:  positive_review → needs_work 

comment:13 Changed 7 years ago by
Commit:  fa2081de3e7b906b60909ed6ea7988ee496d3a59 → 47df94cc7c5181f99717d164b1929c516d039ec1 

Branch pushed to git repo; I updated commit sha1. New commits:
47df94c  Fix conversion of pari(LONG_MIN) to Python int

comment:14 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:16 Changed 7 years ago by
Keywords:  days77 added 

comment:17 followup: 18 Changed 7 years ago by
Would it be possible to test the subtle LONG_MIN
case in gen_to_integer
? Maybe along
sage: a = gen_to_integer(pari("2^631")); 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^631")); 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 followup: 20 Changed 7 years ago by
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
Status:  needs_review → needs_work 

comment:20 followup: 21 Changed 7 years ago by
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 Changed 7 years ago by
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
Reviewers:  Luca De Feo → Luca De Feo, Vincent Delecroix 

comment:23 Changed 7 years ago by
Commit:  47df94cc7c5181f99717d164b1929c516d039ec1 → 8668603e309601df7321762e499c1a2063c35fa3 

Branch pushed to git repo; I updated commit sha1. New commits:
8668603  Add doctests for corner cases

comment:24 Changed 7 years ago by
Status:  needs_work → needs_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 specialcase 32bit vs. 64bit.
comment:25 Changed 7 years ago by
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
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
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
Cc:  JeanPierre Flori added 

comment:29 Changed 7 years ago by
Commit:  8668603e309601df7321762e499c1a2063c35fa3 → b55085547315715f292fd08ce74ad5a437abc6a5 

comment:30 Changed 7 years ago by
I'm now using the flag fnotreecopyrename
which seems to fix all instances of the GCC 4.8 problem.
comment:31 followup: 32 Changed 7 years ago by
Interestingnot 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/gccpatches/201503/msg01460.html
comment:32 Changed 7 years ago by
Replying to embray:
Interestingnot 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 GCC4.9, see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56982
comment:33 followup: 34 Changed 7 years ago by
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 CAPI does not document an alternative way of doing things.
comment:34 Changed 7 years ago by
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
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;}
comment:36 Changed 7 years ago by
Status:  needs_review → needs_work 

I see. Python 3 adds an extra level of indirection ob_base>ob_size
.
comment:37 Changed 7 years ago by
Status:  needs_work → needs_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
Keywords:  days74 added 

Milestone:  sage7.2 → sage7.3 
Status:  needs_review → positive_review 
comment:39 Changed 7 years ago by
Branch:  u/jdemeyer/ticket/20226 → b55085547315715f292fd08ce74ad5a437abc6a5 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Upgrade cysignals package
Move memory functions to cysignals
Rename sage_malloc > sig_malloc and friends
Get rid of factorint_withproof_sage in PARI interface
Stop using deprecated PARI factoring features
Merge branch 't/20205/get_rid_of_factorint_withproof_sage_in_pari_interface' into HEAD
Replace pari_catch_sig_on by sig_on
Deprecate PARI nth_prime and prime_list
Remove redundant functions from pari_instance.pyx
Implement conversion PARI <> Python int/long without mpz