Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py3: some care for integer.pyx #26756

Closed
fchapoton opened this issue Nov 24, 2018 · 18 comments
Closed

py3: some care for integer.pyx #26756

fchapoton opened this issue Nov 24, 2018 · 18 comments

Comments

@fchapoton
Copy link
Contributor

where hex, oct, long, hash are problematic.

Here we take care only of hex and long.

CC: @tscrim @jdemeyer @embray

Component: python3

Author: Frédéric Chapoton

Branch/Commit: 394817c

Reviewer: Travis Scrimshaw, Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/26756

@fchapoton fchapoton added this to the sage-8.5 milestone Nov 24, 2018
@fchapoton
Copy link
Contributor Author

New commits:

a98f30bsome fixes and deprecation for hex / oct of Integer

@fchapoton
Copy link
Contributor Author

Author: Frédéric Chapoton

@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/26756

@fchapoton
Copy link
Contributor Author

Commit: a98f30b

@fchapoton fchapoton changed the title py3: some care integer.pyx py3: some care for integer.pyx Nov 24, 2018
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 24, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

3b81648fixing doctests for hex and oct in lazy_import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 24, 2018

Changed commit from a98f30b to 3b81648

@tscrim
Copy link
Collaborator

tscrim commented Nov 24, 2018

comment:3

LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Nov 24, 2018

Reviewer: Travis Scrimshaw

@embray
Copy link
Contributor

embray commented Nov 26, 2018

comment:4

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).

@fchapoton
Copy link
Contributor Author

comment:5

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.

@embray
Copy link
Contributor

embray commented Nov 26, 2018

comment:6

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:.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 3, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

b406d22Merge branch 'u/chapoton/26756' in 8.5.b6
394817ctrac 26756 some changes in `__hex__`, `__oct__` for lazy imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 3, 2018

Changed commit from 3b81648 to 394817c

@fchapoton
Copy link
Contributor Author

comment:8

better like this, Erik ?

@embray
Copy link
Contributor

embray commented Dec 5, 2018

comment:9

Okay, thanks!

@embray
Copy link
Contributor

embray commented Dec 5, 2018

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Erik Bray

@vbraun
Copy link
Member

vbraun commented Dec 7, 2018

Changed branch from u/chapoton/26756 to 394817c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants