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: | sage-8.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()
built-ins 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@pc-chapoton:~/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 Cython-level 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