Opened 3 years ago

Closed 3 years ago

#28321 closed defect (fixed)

py3: rationals can not be initialized from a pair of big Python ints

Reported by: Vincent Delecroix Owned by:
Priority: critical Milestone: sage-8.9
Component: python3 Keywords:
Cc: Merged in:
Authors: Vincent Delecroix, Jeroen Demeyer Reviewers: Jeroen Demeyer, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: ef9ae69 (Commits, GitHub, GitLab) Commit: ef9ae6904a4620e36c75866f94cc337d5e524ef2
Dependencies: Stopgaps:

Status badges

Description

sage: QQ((2r**100,3r**100))
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
<ipython-input-2-091103d07f87> in <module>()
----> 1 QQ((2**Integer(100),3**Integer(100)))

/opt/sage/sage-py3-gcc/local/lib/python3.7/site-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9197)()
    898         if mor is not None:
    899             if no_extra_args:
--> 900                 return mor._call_(x)
    901             else:
    902                 return mor._call_with_args(x, args, kwds)

/opt/sage/sage-py3-gcc/local/lib/python3.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4556)()
    160                 print(type(C), C)
    161                 print(type(C._element_constructor), C._element_constructor)
--> 162             raise
    163 
    164     cpdef Element _call_with_args(self, x, args=(), kwds={}):

/opt/sage/sage-py3-gcc/local/lib/python3.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4448)()
    155         cdef Parent C = self._codomain
    156         try:
--> 157             return C._element_constructor(x)
    158         except Exception:
    159             if print_warnings:

/opt/sage/sage-py3-gcc/local/lib/python3.7/site-packages/sage/rings/rational.pyx in sage.rings.rational.Rational.__init__ (build/cythonized/sage/rings/rational.c:6386)()
    520         """
    521         if x is not None:
--> 522             self.__set_value(x, base)
    523 
    524     def __reduce__(self):

/opt/sage/sage-py3-gcc/local/lib/python3.7/site-packages/sage/rings/rational.pyx in sage.rings.rational.Rational.__set_value (build/cythonized/sage/rings/rational.c:7608)()
    615             if isinstance(num, int) and isinstance(denom, int):
    616                 if denom >= 0:
--> 617                     mpq_set_si(self.value, num, denom)
    618                 else:
    619                     mpq_set_si(self.value, -num, -denom)

OverflowError: Python int too large to convert to C long

Change History (12)

comment:1 Changed 3 years ago by Vincent Delecroix

Branch: u/vdelecroix/28321
Commit: 50cce861e438f965a283dfbf6b83d3aa06732559
Status: newneeds_review

New commits:

50cce86fix rational constructor

comment:2 Changed 3 years ago by git

Commit: 50cce861e438f965a283dfbf6b83d3aa06732559ab164da54b408b68f67e194b1403010d56e1fce1

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

ab164dafix rational constructor

comment:3 Changed 3 years ago by Jeroen Demeyer

Component: basic arithmeticpython3
Summary: rationals can not be initialized from a pair of big Python intspy3: rationals can not be initialized from a pair of big Python ints

Clarifying that this is a Python 3 issue.

comment:4 Changed 3 years ago by Jeroen Demeyer

Positive review if this passes tests also on Python 2.

comment:5 Changed 3 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer

comment:6 Changed 3 years ago by Vincent Delecroix

mpz_set_pylong with Python2 seems unhappy when providing int... do you have an idea on how to sort this out?

I just ran into this problem again with #28370.

comment:7 Changed 3 years ago by Jeroen Demeyer

Authors: Vincent DelecroixVincent Delecroix, Jeroen Demeyer

comment:8 Changed 3 years ago by Jeroen Demeyer

Branch: u/vdelecroix/28321u/jdemeyer/28321

comment:9 Changed 3 years ago by Jeroen Demeyer

Commit: ab164da54b408b68f67e194b1403010d56e1fce1ef9ae6904a4620e36c75866f94cc337d5e524ef2

The correct solution is to have separate cases for long and int, keeping in mind that the long type on Python 3 is actually called int.


New commits:

d9e8bc5fix rational constructor
ef9ae69Fix conversion from Python 2 ints to QQ

comment:10 Changed 3 years ago by Vincent Delecroix

I see. Thanks!

comment:11 Changed 3 years ago by Vincent Delecroix

Reviewers: Jeroen DemeyerJeroen Demeyer, Vincent Delecroix
Status: needs_reviewpositive_review

Tests pass with your version!

comment:12 Changed 3 years ago by Volker Braun

Branch: u/jdemeyer/28321ef9ae6904a4620e36c75866f94cc337d5e524ef2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.