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

[chapoton]

as a step towards correct cythonization and py3

[chapoton]

New commits:

​b91273e

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

[jdemeyer]

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

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

[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

[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 ?

[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

[tscrim]

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

[chapoton]

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

[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?

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

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

[chapoton]

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

[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__.

[chapoton]

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

[git]

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

​ca947b9	tracc 22048 trying to use richcmp
​ab3a827	trac 22048 introduce __eq__ and __ne__

[tscrim]

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

[git]

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

​a943b9a

trac 22048 remove useless__richcmp__

[tscrim]

LGTM.