Opened 3 years ago

Closed 3 years ago

#24227 closed enhancement (fixed)

py3: check for long before check for int in some pyx files (part 2)

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

Description


Change History (14)

comment:1 Changed 3 years ago by jdemeyer

  • Branch set to public/longint24221
  • Commit set to 0fb286dda3a0f88c3336153f7996cba676046bc5
  • Dependencies set to #24221
  • Status changed from new to needs_review

New commits:

190a90cCheck "long" before "int"
0fb286dtrac 24221 Eric's suggestions of additional changes

comment:2 follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This change is a bit arbitrary:

@@ -3109,6 +3110,11 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
         cdef Integer z
         cdef long yy, res
 
+        if isinstance(y, long):
+            # This should be treated basically the same as if y is an Integer
+            # so go ahead and convert y to an Integer first
+            y = Integer(y)
+
         # First case: Integer % Integer
         if type(x) is type(y):
             if not mpz_sgn((<Integer>y).value):

First of all, I think it makes sense to do this both for x and for y. Second, could you optimize the Integer() call a bit by using PY_NEW and mpz_set_pylong instead?

comment:3 Changed 3 years ago by chapoton

  • Cc embray added

Erik, could you handle Jeroen's queries ?

comment:4 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by embray

Replying to jdemeyer:

This change is a bit arbitrary:

@@ -3109,6 +3110,11 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
         cdef Integer z
         cdef long yy, res
 
+        if isinstance(y, long):
+            # This should be treated basically the same as if y is an Integer
+            # so go ahead and convert y to an Integer first
+            y = Integer(y)
+
         # First case: Integer % Integer
         if type(x) is type(y):
             if not mpz_sgn((<Integer>y).value):

First of all, I think it makes sense to do this both for x and for y. Second, could you optimize the Integer() call a bit by using PY_NEW and mpz_set_pylong instead?

I could be missing some subtlety here but why would you do "this" for x? This is in Integer.__mod__ so x would never be a long.

comment:5 Changed 3 years ago by git

  • Commit changed from 0fb286dda3a0f88c3336153f7996cba676046bc5 to 94127f907492efe915e6231042384d54d9d51a75

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

94127f9Check "long" before "int"

comment:6 follow-up: Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

I incorporated Jeroen's other suggestion and squashed. Awaiting clarification on the first comment.

comment:7 in reply to: ↑ 4 Changed 3 years ago by jdemeyer

Replying to embray:

I could be missing some subtlety here but why would you do "this" for x? This is in Integer.__mod__ so x would never be a long.

This is a difference between the Python language and the C API.

Python has __mod__ and __rmod__, the C API has tp_as_number->nb_remainder where it's not guaranteed that the first argument is of the expected type.

Cython interfaces the C API but with a syntax similar to Python. Most of time, this is useful, but it does cause some confusion. This is probably also the reason why the arguments are called (x, y) instead of (self, other).

comment:8 Changed 3 years ago by embray

Ah, I wondered if something like that might be the case but I wasn't sure. Okay, I'll clean this up a bit more.

comment:9 Changed 3 years ago by jdemeyer

  • Dependencies #24221 deleted
  • Status changed from needs_review to needs_work

I'll split off the __mod__ changes.

comment:10 in reply to: ↑ 6 Changed 3 years ago by jdemeyer

Replying to embray:

I incorporated Jeroen's other suggestion and squashed.

Don't squash! The first commit came from the dependency #24221. Let me try to fix this mess...

comment:11 Changed 3 years ago by git

  • Commit changed from 94127f907492efe915e6231042384d54d9d51a75 to 5f9e545b968525d9b08b9698ba0d589bac7759c8

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

5f9e545Check "long" before "int"

comment:12 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to positive_review

comment:13 Changed 3 years ago by jdemeyer

I'll redo Integer.__mod__ on #24293.

comment:14 Changed 3 years ago by vbraun

  • Branch changed from public/longint24221 to 5f9e545b968525d9b08b9698ba0d589bac7759c8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.