#28321 closed defect (fixed)

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

Reported by: vdelecroix 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 21 months ago by vdelecroix

  • Branch set to u/vdelecroix/28321
  • Commit set to 50cce861e438f965a283dfbf6b83d3aa06732559
  • Status changed from new to needs_review

New commits:

50cce86fix rational constructor

comment:2 Changed 21 months ago by git

  • Commit changed from 50cce861e438f965a283dfbf6b83d3aa06732559 to ab164da54b408b68f67e194b1403010d56e1fce1

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

ab164dafix rational constructor

comment:3 Changed 21 months ago by jdemeyer

  • Component changed from basic arithmetic to python3
  • Summary changed from rationals can not be initialized from a pair of big Python ints to py3: rationals can not be initialized from a pair of big Python ints

Clarifying that this is a Python 3 issue.

comment:4 Changed 21 months ago by jdemeyer

Positive review if this passes tests also on Python 2.

comment:5 Changed 21 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:6 Changed 20 months ago by vdelecroix

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 20 months ago by jdemeyer

  • Authors changed from Vincent Delecroix to Vincent Delecroix, Jeroen Demeyer

comment:8 Changed 20 months ago by jdemeyer

  • Branch changed from u/vdelecroix/28321 to u/jdemeyer/28321

comment:9 Changed 20 months ago by jdemeyer

  • Commit changed from ab164da54b408b68f67e194b1403010d56e1fce1 to ef9ae6904a4620e36c75866f94cc337d5e524ef2

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 20 months ago by vdelecroix

I see. Thanks!

comment:11 Changed 20 months ago by vdelecroix

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Vincent Delecroix
  • Status changed from needs_review to positive_review

Tests pass with your version!

comment:12 Changed 20 months ago by vbraun

  • Branch changed from u/jdemeyer/28321 to ef9ae6904a4620e36c75866f94cc337d5e524ef2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.