Opened 4 years ago

Closed 4 years ago

#24931 closed enhancement (fixed)

py3: get rid of __cmp__ in real_mpfr.pyx

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: b138fe5 (Commits, GitHub, GitLab) Commit: b138fe51eb9302d95b7c4287ebe4510e77bbca39
Dependencies: Stopgaps:

Status badges

Description

part of #16537

Change History (19)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/24931
  • Commit set to ec67c243b6947ef72514352d582eee0533859333
  • Status changed from new to needs_review

New commits:

ec67c24get rid of __cmp__ in real_mpfr.pyx

comment:2 Changed 4 years ago by tscrim

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

LGTM.

comment:3 Changed 4 years ago by chapoton

  • Status changed from positive_review to needs_work

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

comment:4 Changed 4 years ago by 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

comment:5 follow-up: Changed 4 years ago by 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.

comment:6 Changed 4 years ago by embray

I have:

  • src/sage/rings/real_mpfr.pyx

    diff --git a/src/sage/rings/real_mpfr.pyx b/src/sage/rings/real_mpfr.pyx
    index 91d8ad1..e13efa0 100644
    a b cdef class RealField_class(sage.rings.ring.Field): 
    737738            return self.convert_method_map(S, "_mpfr_")
    738739        return self._coerce_map_via([RLF], S)
    739740
    740     def __cmp__(self, other):
     741    def __eq__(self, other):
    741742        """
    742743        Compare two real fields, returning ``True`` if they are equivalent
    743744        and ``False`` if they are not.
    cdef class RealField_class(sage.rings.ring.Field): 
    748749            False
    749750            sage: RealField(10) == RealField(10)
    750751            True
    751             sage: RealField(10,rnd='RNDN') == RealField(10,rnd='RNDZ')
     752            sage: RealField(10, rnd='RNDN') == RealField(10, rnd='RNDZ')
    752753            False
    753754
    754755        Scientific notation affects only printing, not mathematically how
    755756        the field works, so this does not affect equality testing::
    756757
    757             sage: RealField(10,sci_not=True) == RealField(10,sci_not=False)
     758            sage: RealField(10, sci_not=True) == RealField(10, sci_not=False)
    758759            True
    759760            sage: RealField(10) == IntegerRing()
    760761            False
    cdef class RealField_class(sage.rings.ring.Field): 
    769770            True
    770771        """
    771772        if not isinstance(other, RealField_class):
    772             return -1
     773            return False
     774
    773775        cdef RealField_class _other
    774776        _other = other  # to access C structure
    775         if self.__prec == _other.__prec and self.rnd == _other.rnd: \
    776                #and self.sci_not == _other.sci_not:
    777             return 0
    778         return 1
     777        return self.__prec == _other.__prec and self.rnd == _other.rnd

comment:7 in reply to: ↑ 5 Changed 4 years ago by 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).

comment:8 Changed 4 years ago by git

  • Commit changed from ec67c243b6947ef72514352d582eee0533859333 to f7cfcebdce9a9d9845b88a546616174f80ada9bd

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

f7cfcebtrac 24931 enhancements and corrections

comment:9 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:10 Changed 4 years ago by embray

  • Status changed from needs_review to needs_work

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)

comment:11 Changed 4 years ago by git

  • Commit changed from f7cfcebdce9a9d9845b88a546616174f80ada9bd to b138fe51eb9302d95b7c4287ebe4510e77bbca39

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

b4cc1f5Merge branch 'u/chapoton/24931' in 8.2.b8
b138fe5trac 23931 detail

comment:12 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

thanks, done

comment:13 follow-up: Changed 4 years ago by 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?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by 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.

Last edited 4 years ago by embray (previous) (diff)

comment:15 Changed 4 years ago by chapoton

so, positive review ?

comment:16 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by 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?

comment:17 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:18 in reply to: ↑ 16 Changed 4 years ago by 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).

comment:19 Changed 4 years ago by vbraun

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