Opened 8 months ago

Closed 8 months 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) Commit: 394817c666c8bab73711c859cf047cab2b1517a1
Dependencies: Stopgaps:

Description (last modified by chapoton)

where hex, oct, long, hash are problematic.

Here we take care only of hex and long.

Change History (10)

comment:1 Changed 8 months ago by chapoton

  • Authors set to Frédéric Chapoton
  • 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

New commits:

​a98f30bsome fixes and deprecation for hex / oct of Integer

comment:2 Changed 8 months ago by git

  • Commit changed from a98f30b59a2bc9703cf5033b41a1b4e9d7baa199 to 3b816484ef88b66a514f5c5114546d29309391e4

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

​3b81648fixing doctests for hex and oct in lazy_import

comment:3 Changed 8 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:4 Changed 8 months ago by embray

  • 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 8 months ago by chapoton

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 8 months ago by embray

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

Last edited 8 months ago by embray (previous) (diff)

comment:7 Changed 8 months ago by git

  • Commit changed from 3b816484ef88b66a514f5c5114546d29309391e4 to 394817c666c8bab73711c859cf047cab2b1517a1

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

comment:8 Changed 8 months ago by chapoton

  • Status changed from needs_work to needs_review

better like this, Erik ?

comment:9 Changed 8 months ago by embray

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Erik Bray
  • Status changed from needs_review to positive_review

Okay, thanks!

comment:10 Changed 8 months ago by vbraun

  • Branch changed from u/chapoton/26756 to 394817c666c8bab73711c859cf047cab2b1517a1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.