Opened 3 years ago
Closed 3 years ago
#26756 closed enhancement (fixed)
py3: some care for integer.pyx
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  python3  Keywords:  
Cc:  tscrim, jdemeyer, embray  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw, Erik Bray 
Report Upstream:  N/A  Work issues:  
Branch:  394817c (Commits, GitHub, GitLab)  Commit:  394817c666c8bab73711c859cf047cab2b1517a1 
Dependencies:  Stopgaps: 
Description (last modified by )
where hex, oct, long, hash are problematic.
Here we take care only of hex and long.
Change History (10)
comment:1 Changed 3 years ago by
 Branch set to u/chapoton/26756
 Cc tscrim jdemeyer embray added
 Commit set to a98f30b59a2bc9703cf5033b41a1b4e9d7baa199
 Description modified (diff)
 Status changed from new to needs_review
 Summary changed from py3: some care integer.pyx to py3: some care for integer.pyx
comment:2 Changed 3 years ago by
 Commit changed from a98f30b59a2bc9703cf5033b41a1b4e9d7baa199 to 3b816484ef88b66a514f5c5114546d29309391e4
Branch pushed to git repo; I updated commit sha1. New commits:
3b81648  fixing doctests for hex and oct in lazy_import

comment:3 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:4 Changed 3 years ago by
 Status changed from positive_review to needs_work
The changes to LazyImport
are not right. I agree with not using the hex()
and oct()
builtins here. Rather, it should just pass to self.get_object.__hex__()
and __oct__()
respectively; there's no guarantee that the proxied object has .oct()
or .hex()
methods.
This is meant to be pretty a pretty generic proxy class (in fact I have a ticket I would like to revive sometime where I reimplement this on an existing proxy library for Python: #22793).
comment:5 Changed 3 years ago by
Hello Erik. Glad to see you back. Would you please look at #26740 ?
Here are the only remaining __hex__
in sage:
chapoton@pcchapoton:~/sage$ git grep "def __hex__(" src/sage src/sage/libs/ntl/ntl_GF2X.pyx: def __hex__(self): src/sage/misc/lazy_import.pyx: def __hex__(self): src/sage/rings/integer.pyx: def __hex__(self): src/sage/rings/real_mpfr.pyx: def __hex__(self):
and we also have
src/sage/libs/ntl/ntl_GF2X.pyx: def hex(ntl_GF2X self): src/sage/rings/real_mpfr.pyx: def hex(self):
I think the __hex__
should all disappear. In python3, hex
call the __index__
method.
comment:6 Changed 3 years ago by
I never went away really. I've just been working on other stuff (mainly related to GAP). But I thought I should try to take a day or two to catch up on other things :)
That's a good point. You could also leave __hex__
and __oct__
as they were previously, but they should be bracketed with a Cythonlevel if like IF PY_MAJOR_VERSION < 3:
.
comment:7 Changed 3 years ago by
 Commit changed from 3b816484ef88b66a514f5c5114546d29309391e4 to 394817c666c8bab73711c859cf047cab2b1517a1
comment:8 Changed 3 years ago by
 Status changed from needs_work to needs_review
better like this, Erik ?
comment:9 Changed 3 years ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Erik Bray
 Status changed from needs_review to positive_review
Okay, thanks!
comment:10 Changed 3 years ago by
 Branch changed from u/chapoton/26756 to 394817c666c8bab73711c859cf047cab2b1517a1
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
some fixes and deprecation for hex / oct of Integer