Opened 3 years ago

Closed 3 years ago

#24221 closed enhancement (fixed)

py3: ZZ for large int

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.1
Component: python3 Keywords:
Cc: jdemeyer, embray Merged in:
Authors: Jeroen Demeyer Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 190a90c (Commits) Commit: 190a90c048520bb954d1a41757f646f9c3ec8b30
Dependencies: Stopgaps:

Description (last modified by chapoton)

in a python3-build-sage:

sage: ZZ(int(2**62))
4611686018427387904
sage: ZZ(int(2**63))
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
OverflowError: Python int too large to convert to C long

The above exception was the direct cause of the following exception:

SystemError                               Traceback (most recent call last)
<ipython-input-41-47ff15a43d5f> in <module>()
----> 1 ZZ(int(Integer(2)**Integer(64)))

SystemError: Integer Ring returned a result with an error set
sage: int(2**63)
18446744073709551616

related to #16072

Change History (19)

comment:1 Changed 3 years ago by chapoton

  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

Is the text that you quoted above the literal text that you see on your screen? Just wondering why there is no traceback shown...

comment:3 Changed 3 years ago by chapoton

yes, this the literal text that I see in my terminal. No better traceback.

By the way, maybe you should try by yourself to use the branch public/python3-experiment-8.1.rc1 ?

comment:4 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/py3__zz_for_large_int

comment:5 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to f27e59b360c8a73d23cb4e108922197afbaa00ac
  • Status changed from new to needs_review

There is a general pattern here:

In Cython code in Sage, when checking for an int-like object, this is often done like

if isinstance(x, int):
    # Handle int
elif isinstance(x, long):
    # Handle long

This works fine in Python 2 because int and long are distinct types. In Python 3, there is only one type, which is the long type from Python 2 but which is called int in Python 3 (similar to how the unicode type from Python 2 is called str in Python 3).

Now Cython understands the name long to refer to the type which is called long in Python 2 and int in Python 3. It also understands int to refer to the type which is called int in Python 2 and the different type which is called int in Python 3.

So, in Cython code for Python 3, the checks isinstance(x, int) and isinstance(x, long) are equivalent. However, the code which needs to be executed is the code for the long type, so we should do that check first.


New commits:

f27e59bCheck "long" before "int"

comment:6 Changed 3 years ago by chapoton

This does not fix the issue:

sage: ZZ(int(2**63))
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
OverflowError: Python int too large to convert to C long

The above exception was the direct cause of the following exception:

SystemError                               Traceback (most recent call last)
<ipython-input-3-9949e9fafd8e> in <module>()
----> 1 ZZ(int(Integer(2)**Integer(63)))

SystemError: Integer Ring returned a result with an error set

comment:7 Changed 3 years ago by jdemeyer

Sorry, I can't help further without a traceback.

comment:8 Changed 3 years ago by git

  • Commit changed from f27e59b360c8a73d23cb4e108922197afbaa00ac to 190a90c048520bb954d1a41757f646f9c3ec8b30

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

190a90cCheck "long" before "int"

comment:9 Changed 3 years ago by jdemeyer

Never mind, I just realized that the example uses coercion, not direct initialization of an Integer.

comment:10 Changed 3 years ago by chapoton

Indeed

sage: ZZ(2**63)
9223372036854775808

does work (at least with you change)

comment:11 Changed 3 years ago by chapoton

That fixes the issue ! Thanks !

comment:12 Changed 3 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

comment:13 Changed 3 years ago by jdemeyer

There will certainly be more places in Sage with the same bug. But the solution should be the same: whenever you want to check int and long, check long first.

comment:14 Changed 3 years ago by jdemeyer

See also #23792

comment:15 Changed 3 years ago by chapoton

follow up in #24225

comment:16 Changed 3 years ago by chapoton

  • Status changed from positive_review to needs_work

ok, let us use the opportuniy here to make some similar changes in the same file

comment:17 Changed 3 years ago by chapoton

  • Branch changed from u/jdemeyer/py3__zz_for_large_int to public/longint24221
  • Cc embray added
  • Commit changed from 190a90c048520bb954d1a41757f646f9c3ec8b30 to 0fb286dda3a0f88c3336153f7996cba676046bc5
  • Status changed from needs_work to needs_review

New commits:

0fb286dtrac 24221 Eric's suggestions of additional changes

comment:18 Changed 3 years ago by jdemeyer

  • Branch changed from public/longint24221 to u/jdemeyer/py3__zz_for_large_int
  • Commit changed from 0fb286dda3a0f88c3336153f7996cba676046bc5 to 190a90c048520bb954d1a41757f646f9c3ec8b30
  • Status changed from needs_review to positive_review

Moving the follow-up changes to #24227.

comment:19 Changed 3 years ago by vbraun

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