Opened 3 years ago

Closed 3 years ago

#22048 closed enhancement (fixed)

py3 remove cmp() in number_field/number_field_element (pyx)

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.6
Component: python3 Keywords:
Cc: tscrim, jdemeyer, aapitzsch Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a943b9a (Commits) Commit: a943b9ab0b07e5d2bbccc9cfea6523c37b143fce
Dependencies: Stopgaps:

Description

as a step towards correct cythonization and py3

Change History (21)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/22048
  • Cc tscrim jdemeyer aapitzsch added
  • Commit set to b91273eb1952577d423e8b2b689a5b7030b741f0
  • Status changed from new to needs_review

New commits:

b91273etrac #22048 py3 remove one cmp() in pyx file

comment:2 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_info

What is the point of removing cmp() while keeping __cmp__?

comment:3 follow-up: Changed 3 years ago by chapoton

As I may have already explained, my current aim is to be able to cythonize all our pyx files. For this, I only need to get rid of cmp() in a reasonable way. Same reason for tickets #22013 and #22019.

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

I think it makes little sense to change the same comparison function twice: once to get rid of cmp() and once to change __cmp__ to __richcmp__. That seems like more work without much reason.

Replying to chapoton:

my current aim is to be able to cythonize all our pyx files.

If that is really your goal, I would just implement a "fake" cmp() function like this:

cdef int cmp(x, y) except -2:
    if x == y:
        return 0
    elif x < y:
        return -1
    else:
        return 1

comment:5 follow-up: Changed 3 years ago by chapoton

I would prefer to avoid re-creating cmp, but I can bend if it is required.

In which file should I put this "custom cmp" function ?

comment:6 Changed 3 years ago by chapoton

I tried to used __richcmp__ here, but this breaks pickling. Do you have any idea why ? Maybe something like "Non-identical instances of a class normally compare as non-equal unless the class defines the __eq__() method." ? Can this be solved ?

**********************************************************************
File "src/sage/rings/number_field/number_field_element.pyx", line 5000, in sage.rings.number_field.number_field_element.CoordinateFunction
Failed example:
    f == loads(dumps(f))
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/rings/number_field/number_field_element.pyx", line 5068, in sage.rings.number_field.number_field_element.CoordinateFunction.__richcmp__
Failed example:
    f == loads(dumps(f))
Expected:
    True
Got:
    False

comment:7 Changed 3 years ago by tscrim

Can you push your branch where you do __richcmp__ (not necessarily setting the branch field of this ticket of course)?

comment:8 Changed 3 years ago by chapoton

a branch with richmp is available at u/chapoton/experiment_22048

comment:9 follow-up: Changed 3 years ago by tscrim

So my guess is that Cython tries to find a __cmp__, and if it doesn't, then (unlike Python) does not use __richcmp__. For proper Element classes, then there is a default __cmp__, which redirects appropriately to _richcmp_.

However, I'm not sure if this could be considered a bug of Cython, if it something we just have to work around (temporarily until Cython gets this as a feature), or we just have __cmp__ in Cython classes because it is a different language. Jeroen, any comments?

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

Replying to tscrim:

So my guess is that Cython tries to find a __cmp__, and if it doesn't, then (unlike Python) does not use __richcmp__.

It's not "Cython" which has to implement that: it's Python who decides which comparison function to call. So, it cannot be a bug in Cython.

comment:11 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

Replying to chapoton:

In which file should I put this "custom cmp" function ?

I would put it in sage_object.pxd where we already have various functions involving comparisons. However, I don't know exactly how we can ensure that this is only used in Python 3.

Maybe a simpler solution would be to patch Cython such that it does recognize cmp() in Python 3.

comment:12 Changed 3 years ago by chapoton

so what is the conclusion for the ticket here ? should I just implement __eq__ ?

comment:13 Changed 3 years ago by tscrim

Ah, I see my misunderstanding, it is precisely because it is a Python class, not a cdef Cython class why __richcmp__ is not called. So, I'm thinking that is what needs to happen: we should implement a __eq__ and __ne__.

comment:14 Changed 3 years ago by chapoton

Once I introduced __eq__ and __ne__, do I need to keep the __richcmp__ or is it completely useless ?

comment:15 Changed 3 years ago by git

  • Commit changed from b91273eb1952577d423e8b2b689a5b7030b741f0 to ab3a82771ef9b6023c55a79d9e47d5de3cace1dd

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

ca947b9tracc 22048 trying to use richcmp
ab3a827trac 22048 introduce __eq__ and __ne__

comment:16 Changed 3 years ago by chapoton

  • Status changed from needs_info to needs_review

comment:17 Changed 3 years ago by tscrim

The __richcmp__ is something that is only used by cdef classes, so in this case it is useless.

comment:18 Changed 3 years ago by git

  • Commit changed from ab3a82771ef9b6023c55a79d9e47d5de3cace1dd to a943b9ab0b07e5d2bbccc9cfea6523c37b143fce

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

a943b9atrac 22048 remove useless__richcmp__

comment:19 Changed 3 years ago by tscrim

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

LGTM.

comment:20 Changed 3 years ago by chapoton

  • Milestone changed from sage-7.5 to sage-7.6

comment:21 Changed 3 years ago by vbraun

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