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, GitHub, GitLab) | Commit: | 190a90c048520bb954d1a41757f646f9c3ec8b30 |
Dependencies: | Stopgaps: |
Description (last modified by )
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
- Description modified (diff)
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
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
- Branch set to u/jdemeyer/py3__zz_for_large_int
comment:5 Changed 3 years ago by
- 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:
f27e59b | Check "long" before "int"
|
comment:6 Changed 3 years ago by
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
Sorry, I can't help further without a traceback.
comment:8 Changed 3 years ago by
- Commit changed from f27e59b360c8a73d23cb4e108922197afbaa00ac to 190a90c048520bb954d1a41757f646f9c3ec8b30
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
190a90c | Check "long" before "int"
|
comment:9 Changed 3 years ago by
Never mind, I just realized that the example uses coercion, not direct initialization of an Integer
.
comment:10 Changed 3 years ago by
Indeed
sage: ZZ(2**63) 9223372036854775808
does work (at least with you change)
comment:11 Changed 3 years ago by
That fixes the issue ! Thanks !
comment:12 Changed 3 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
comment:13 Changed 3 years ago by
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
See also #23792
comment:15 Changed 3 years ago by
follow up in #24225
comment:16 Changed 3 years ago by
- 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
- 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:
0fb286d | trac 24221 Eric's suggestions of additional changes
|
comment:18 Changed 3 years ago by
- 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
- Branch changed from u/jdemeyer/py3__zz_for_large_int to 190a90c048520bb954d1a41757f646f9c3ec8b30
- Resolution set to fixed
- Status changed from positive_review to closed
Is the text that you quoted above the literal text that you see on your screen? Just wondering why there is no traceback shown...