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:  sage8.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: 
Description
part of #16537
Change History (19)
comment:1 Changed 4 years ago by
 Branch set to u/chapoton/24931
 Commit set to ec67c243b6947ef72514352d582eee0533859333
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:3 Changed 4 years ago by
 Status changed from positive_review to needs_work
Hmm, I suspect that one doctest is failing...
comment:4 Changed 4 years ago by
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 followup: ↓ 7 Changed 4 years ago by
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
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): 737 738 return self.convert_method_map(S, "_mpfr_") 738 739 return self._coerce_map_via([RLF], S) 739 740 740 def __ cmp__(self, other):741 def __eq__(self, other): 741 742 """ 742 743 Compare two real fields, returning ``True`` if they are equivalent 743 744 and ``False`` if they are not. … … cdef class RealField_class(sage.rings.ring.Field): 748 749 False 749 750 sage: RealField(10) == RealField(10) 750 751 True 751 sage: RealField(10, rnd='RNDN') == RealField(10,rnd='RNDZ')752 sage: RealField(10, rnd='RNDN') == RealField(10, rnd='RNDZ') 752 753 False 753 754 754 755 Scientific notation affects only printing, not mathematically how 755 756 the field works, so this does not affect equality testing:: 756 757 757 sage: RealField(10, sci_not=True) == RealField(10,sci_not=False)758 sage: RealField(10, sci_not=True) == RealField(10, sci_not=False) 758 759 True 759 760 sage: RealField(10) == IntegerRing() 760 761 False … … cdef class RealField_class(sage.rings.ring.Field): 769 770 True 770 771 """ 771 772 if not isinstance(other, RealField_class): 772 return 1 773 return False 774 773 775 cdef RealField_class _other 774 776 _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
comment:8 Changed 4 years ago by
 Commit changed from ec67c243b6947ef72514352d582eee0533859333 to f7cfcebdce9a9d9845b88a546616174f80ada9bd
Branch pushed to git repo; I updated commit sha1. New commits:
f7cfceb  trac 24931 enhancements and corrections

comment:9 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:10 Changed 4 years ago by
 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
 Commit changed from f7cfcebdce9a9d9845b88a546616174f80ada9bd to b138fe51eb9302d95b7c4287ebe4510e77bbca39
comment:13 followup: ↓ 14 Changed 4 years ago by
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 rightside 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 ; followup: ↓ 16 Changed 4 years ago by
Replying to embray:
If
IntegerRing_class.__richcmp__
also contained something likeif not isinstance(right, IntegerRing_class): return NotImplemented
, and then you attempted the aforementioned comparison, instead of a boolean result you'll get aTypeError
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.
comment:15 Changed 4 years ago by
so, positive review ?
comment:16 in reply to: ↑ 14 ; followup: ↓ 18 Changed 4 years ago by
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
 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
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
 Branch changed from u/chapoton/24931 to b138fe51eb9302d95b7c4287ebe4510e77bbca39
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
get rid of __cmp__ in real_mpfr.pyx