Opened 2 years ago

Closed 2 years ago

#24225 closed enhancement (fixed)

py3: check for long before check for int in some pyx files

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.1
Component: python3 Keywords:
Cc: embray, jdemeyer, tscrim, fbissey Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: dd3d657 (Commits) Commit: dd3d657688439a87c5d3fbc1b6a308a541383b03
Dependencies: Stopgaps:

Description

as in #24221

Change History (17)

comment:1 Changed 2 years ago by chapoton

  • Branch set to u/chapoton/24225
  • Status changed from new to needs_review

comment:2 Changed 2 years ago by git

  • Commit set to 542989719c96c40f91eb68f67a276c52b8ce7a80

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

5429897py3: check for long before check for int in some pyx files

comment:3 Changed 2 years ago by jdemeyer

Syntax error in src/sage/libs/mpmath/ext_impl.pyx

comment:4 Changed 2 years ago by jdemeyer

The int() call in plural.pyx requires further thought. Can you revert that for now?

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:5 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:6 Changed 2 years ago by git

  • Commit changed from 542989719c96c40f91eb68f67a276c52b8ce7a80 to dd3d657688439a87c5d3fbc1b6a308a541383b03

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

dd3d657py3: check for long before check for int in some pyx files

comment:7 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

Done, thanks.

comment:8 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

Good for me if the (Python 2) patchbot passes.

comment:9 Changed 2 years ago by embray

Ah, I just fixed a bunch of these myself :) Glad I don't have to make a ticket for it now.

I might add a comment, at least a few of these cases, making it clear that the isinstance(..., int) case should never run on Python 3, since this is somewhat non-obvious, at least to me.

comment:10 Changed 2 years ago by embray

Spoke too soon. Here's a couple more if you want to add them to this ticket; otherwise I can open a new one...

  • src/sage/rings/integer.pyx

    diff --git a/src/sage/rings/integer.pyx b/src/sage/rings/integer.pyx
    index 7e86b95..ff186cc 100644
    a b cdef class Integer(sage.structure.element.EuclideanDomainEleme 
    926929            c = mpz_cmp((<Integer>left).value, (<Integer>right).value)
    927930        elif isinstance(right, Rational):
    928931            c = -mpq_cmp_z((<Rational>right).value, (<Integer>left).value)
    929         elif isinstance(right, int):
    930             c = mpz_cmp_si((<Integer>left).value, PyInt_AS_LONG(right))
    931932        elif isinstance(right, long):
    932933            mpz_init(mpz_tmp)
    933934            mpz_set_pylong(mpz_tmp, right)
    934935            c = mpz_cmp((<Integer>left).value, mpz_tmp)
    935936            mpz_clear(mpz_tmp)
     937        elif isinstance(right, int):
     938            # This case should only occur on Python 2
     939            c = mpz_cmp_si((<Integer>left).value, PyInt_AS_LONG(right))
    936940        elif isinstance(right, float):
    937941            d = right
    938942            if isnan(d):
    cdef class Integer(sage.structure.element.EuclideanDomainElem 
    31153119        cdef Integer z
    31163120        cdef long yy, res
    31173121
     3122        if isinstance(y, long):
     3123            # This should be treated basically the same as if y is an Integer
     3124            # so go ahead and convert y to an Integer first
     3125            y = Integer(y)
     3126
    31183127        # First case: Integer % Integer
    31193128        if type(x) is type(y):
    31203129            if not mpz_sgn((<Integer>y).value):

Edit: Originally a few of these diffs were the same as in #24221, but I think the ones here now are still new...

Last edited 2 years ago by embray (previous) (diff)

comment:11 Changed 2 years ago by chapoton

Those are done in #24221

oh, I missed your edit..

Last edited 2 years ago by chapoton (previous) (diff)

comment:12 Changed 2 years ago by chapoton

maybe we should rather add them to #24221 to minimize conflicts

comment:13 follow-up: Changed 2 years ago by chapoton

I have added your additional changes to #24221, that is back to "needs review"

comment:14 Changed 2 years ago by chapoton

green bot here

comment:15 in reply to: ↑ 13 Changed 2 years ago by jdemeyer

Replying to chapoton:

I have added your additional changes to #24221

Moved that to #24227 instead.

comment:16 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:17 Changed 2 years ago by vbraun

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