py3: get rid of __cmp__ in real_mpfr.pyx

[chapoton]

part of #16537

[chapoton]

New commits:

​ec67c24

get rid of __cmp__ in real_mpfr.pyx

[tscrim]

LGTM.

[chapoton]

Hmm, I suspect that one doctest is failing...

[jdemeyer]

Some comments from #24930 apply here too.

And it seems that the __richcmp__ is really only meant to check for equality, so I would add

if op != Py_EQ and op != Py_NE:
    return NotImplemented

[embray]

Indeed. I don't think you even need the full __richcmp__. I implemented this just as an __eq__, but for that to work requires Cython 0.28 or a patched Cython.

[embray]

I have:

src/sage/rings/real_mpfr.pyx

@@ -737 +738 @@
             return self.convert_method_map(S, "_mpfr_")
         return self._coerce_map_via([RLF], S)
 
-    def __cmp__(self, other):
+    def __eq__(self, other):
         """
         Compare two real fields, returning ``True`` if they are equivalent
         and ``False`` if they are not.
@@ -748 +749 @@
             False
             sage: RealField(10) == RealField(10)
             True
-            sage: RealField(10,rnd='RNDN') == RealField(10,rnd='RNDZ')
+            sage: RealField(10, rnd='RNDN') == RealField(10, rnd='RNDZ')
             False
 
         Scientific notation affects only printing, not mathematically how
         the field works, so this does not affect equality testing::
 
-            sage: RealField(10,sci_not=True) == RealField(10,sci_not=False)
+            sage: RealField(10, sci_not=True) == RealField(10, sci_not=False)
             True
             sage: RealField(10) == IntegerRing()
             False
@@ -769 +770 @@
             True
         """
         if not isinstance(other, RealField_class):
-            return -1
+            return False
+
         cdef RealField_class _other
         _other = other  # to access C structure
-        if self.__prec == _other.__prec and self.rnd == _other.rnd: \
-               #and self.sci_not == _other.sci_not:
-            return 0
-        return 1
+        return self.__prec == _other.__prec and self.rnd == _other.rnd

[jdemeyer]

Replying to embray:

Indeed. I don't think you even need the full __richcmp__. I implemented this just as an __eq__, but for that to work requires Cython 0.28 or a patched Cython.

FYI: Cython 0.28 is in beta now (#24111).

[git]

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

​f7cfceb

trac 24931 enhancements and corrections

[embray]

It doesn't make sense to use rich_to_bool here at all. All you need is return (self.__prec == _other.__prec and self.rnd == _other.rnd) == (op == Py_EQ)

[git]

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

​b4cc1f5	Merge branch 'u/chapoton/24931' in 8.2.b8
​b138fe5	trac 23931 detail

[chapoton]

thanks, done

[embray]

Right now you have:

if not isinstance(other, RealField_class):
    return NotImplemented

which I think was Jeroen's suggestion. The reason for this is that it will delegate comparison to the class of the right-side object if it is not an instance of RealField_class. So that sort of makes sense. But I'm concerned about making that the general approach.

For example, the tests contain:

    sage: RealField(10) == IntegerRing()
    False

which currently will work. But why, then, isn't the same logic applied in IntegerRing? Currently its __richcmp__ is (somewhat arbitrarily?) implemented rather differently:

    def __richcmp__(left, right, int op):
        """
        Rich comparison of ``left`` and ``right``.

        TESTS::

            sage: from sage.rings.integer_ring import IntegerRing_class
            sage: ZZ == ZZ
            True
            sage: ZZ != QQ
            True
        """
        if left is right:
            return rich_to_bool(op, 0)

        if isinstance(right, IntegerRing_class):
            return rich_to_bool(op, 0)

        if isinstance(right, sage.rings.rational_field.RationalField):
            return rich_to_bool(op, -1)

        return op == Py_NE

That sort of grew out of the old implementation of IntegerRing_class._cmp_, but I don't think this is really right either. Just as in the case of RealField_class, this seems to mostly just care about equality (though it seems it should also return less than RationalField? What's that about?).

If IntegerRing_class.__richcmp__ also contained something like if not isinstance(right, IntegerRing_class): return NotImplemented, and then you attempted the aforementioned comparison, instead of a boolean result you'll get a TypeError exception.

So my question becomes, should a class always delegate rich comparison to the other object, and if not when should it and when shouldn't it?

[embray]

Replying to embray:

If IntegerRing_class.__richcmp__ also contained something like if not isinstance(right, IntegerRing_class): return NotImplemented, and then you attempted the aforementioned comparison, instead of a boolean result you'll get a TypeError exception.

I'm sorry, I was wrong here (though I cannot for the life of me find where this is actually documented). If both a.__eq__(b) and b.__eq__(a) return NotImplemented, then Python falls back on the default comparison which is by id(). I think that's also the case (in reverse) for __ne__. It's only in the case of the ordering operators where it raises TypeError.

I think that clarifies things for me then. We should probably change IntegerRing_class.__richcmp__ to be more consistent, but this is fine as is.

Every time I think I understand Python's rich comparison it turns out I've forgotten half of what I'd learned.

[chapoton]

so, positive review ?

[jdemeyer]

Replying to embray:

Every time I think I understand Python's rich comparison it turns out I've forgotten half of what I'd learned.

Maybe it's also because it works quite different in Python 2 and Python 3?

[embray]

Replying to jdemeyer:

Replying to embray:

Every time I think I understand Python's rich comparison it turns out I've forgotten half of what I'd learned.

Maybe it's also because it works quite different in Python 2 and Python 3?

It does, and it's hard to keep straight. I thought the fallback behavior for equality, for example, was only on Python 2 (when in fact I was probably just thinking about the fallback for ordered comparisons).