Opened 4 years ago

Closed 4 years ago

#24930 closed enhancement (fixed)

py3: some cmp removal in pyx files

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords: python3, richcmp
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 47c7694 (Commits, GitHub, GitLab) Commit: 47c7694486866fad2c7bebd7127a7239864a6b04
Dependencies: Stopgaps:

Status badges

Description

part of #16537

Change History (15)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/24930
  • Commit set to 6dd8b130782889c1fbc5cf46f654742e4304bf61
  • Status changed from new to needs_review

New commits:

6dd8b13some __cmp__ removal in pyx files

comment:2 Changed 4 years ago by chapoton

  • Cc python3 richcmp added

comment:3 Changed 4 years ago by chapoton

  • Cc python3 richcmp removed
  • Keywords python3 richcmp added

comment:4 Changed 4 years ago by tscrim

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

LGTM.

comment:5 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:6 Changed 4 years ago by jdemeyer

  1. In __richcmp__, you really should deal with the case of different classes. Typically, this is done by adding
    if not isinstance(other, TheClassThatYouWant):
        return NotImplemented
    

Otherwise, things might break sooner or later (see #19628 for an example of such breakage).

  1. Once you did check the class with isinstance(), you can do a cast. So replace
    cdef TheClassThatYouWant x = other
    

by

x = <TheClassThatYouWant>other

(you can still add cdef TheClassThatYouWant but that is redundant).

  1. In the light of Cython 0.28, I would prefer to see
    def __richcmp__(TheClassThatYouWant self, other, int op)
        ...
    

instead of

def __richcmp__(self, other, int op)
    cdef TheClassThatYouWant s = self

Ideally, Cython should know that self is of type TheClassThatYouWant. But this is not the case due to a Cython bug which is fixed in version 0.28.

comment:7 Changed 4 years ago by jdemeyer

  1. I believe that the code in intlist.pyx is actually wrong. The second argument to rich_to_bool() must be -1, 0 or 1. If you want to support other integers, you need rich_to_bool_sgn().

comment:8 Changed 4 years ago by embray

FWIW this was my fix for intlist from my Python 3 branch:

  • src/sage/stats/intlist.pyx

    diff --git a/src/sage/stats/intlist.pyx b/src/sage/stats/intlist.pyx
    index 6076475..c6d3f8f 100644
    a b from libc.string cimport memcpy 
    3232from cysignals.memory cimport sig_malloc, sig_free
    3333from cysignals.signals cimport sig_on, sig_off
    3434
     35from sage.structure.richcmp import rich_to_bool
    3536from sage.rings.integer import Integer
    3637from sage.finance.time_series cimport TimeSeries
    3738from cpython.bytes cimport PyBytes_FromStringAndSize, PyBytes_AsString
    cdef class IntList: 
    116117            for i in range(self._length):
    117118                self._values[i] = values[i]
    118119
    119     def __cmp__(self, _other):
     120    def __richcmp__(left, right, op):
    120121        """
    121122        Compare self and other.  This has the same semantics
    122123        as list comparison.
    cdef class IntList: 
    133134            sage: w == w
    134135            True
    135136        """
    136         cdef IntList other
     137        cdef IntList l, r
    137138        cdef Py_ssize_t c, i
    138139        cdef int d
    139         if not isinstance(_other, IntList):
    140             _other = IntList(_other)
    141         other = _other
    142         for i in range(min(self._length, other._length)):
    143             d = self._values[i] - other._values[i]
    144             if d: return -1 if d < 0 else 1
    145         c = self._length - other._length
    146         if c < 0: return -1
    147         elif c > 0: return 1
    148         return 0
     140        if not isinstance(right, IntList):
     141            right = IntList(right)
     142        l, r = left, right
     143        for i in range(min(l._length, r._length)):
     144            d = l._values[i] - r._values[i]
     145            if d:
     146                return rich_to_bool(op, -1 if d < 0 else 1)
     147        c = l._length - r._length
     148        if c < 0:
     149            return rich_to_bool(op, -1)
     150        elif c > 0:
     151            return rich_to_bool(op, +1)
     152        return rich_to_bool(op, 0)

comment:9 Changed 4 years ago by git

  • Commit changed from 6dd8b130782889c1fbc5cf46f654742e4304bf61 to 47c7694486866fad2c7bebd7127a7239864a6b04

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

47c7694trac 24930 enhancements, corrections

comment:10 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:11 Changed 4 years ago by jdemeyer

There is an rc of Cython 0.28 (#24111), so you might as well wait for that.

comment:12 Changed 4 years ago by chapoton

Well, time is flying, and there is no really good reason to wait..

comment:13 Changed 4 years ago by jdemeyer

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:14 Changed 4 years ago by embray

It looks good to me--we can revisit some of the __richcmp__ -> __eq__ rewrites once we can fully depend on Cython 0.28.

comment:15 Changed 4 years ago by vbraun

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