Opened 4 years ago
Closed 4 years ago
#23109 closed enhancement (fixed)
Move richcmp_by_eq_and_lt to structure/richcmp.pyx
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  misc  Keywords:  
Cc:  chapoton, dkrenn  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Daniel Krenn 
Report Upstream:  N/A  Work issues:  
Branch:  aa85807 (Commits, GitHub, GitLab)  Commit:  aa858075bb8dadd839ae5c744b8e81fb82e7f1c3 
Dependencies:  #23102  Stopgaps: 
Description (last modified by )
The function richcmp_by_eq_and_lt
from src/sage/rings/asymptotic/misc.py
should be moved to src/sage/structure/richcmp.pyx
.
I would also like to change the semantics slightly: this would be more useful if the method names (i.e. _eq_
and _lt_
) would be an argument like _richcmp_ = richcmp_by_eq_and_lt("_eq_", "_lt_")
. This way, it can also be used with the standard Python methods __eq__
and __lt__
.
Change History (29)
comment:1 in reply to: ↑ description Changed 4 years ago by
comment:2 Changed 4 years ago by
I think this function should never have been put in src/sage/rings/asymptotic
in the first place. When you write general functions, they should be put in a general place, not hidden in src/sage/rings/asymptotic
.
comment:3 Changed 4 years ago by
For this reason, most of the contents of src/sage/rings/asymptotic/misc.py
doesn't belong there. For example, why is transform_category
not a method of Category
? Hiding such general functions tends to encourage reinventing the wheel. Maybe people working on a completely different part of Sage need something like transform_category
. Those people wouldn't know that it exists in rings/asymptotic
, so they would need to reinvent it. Anyway, I'm not going to deal with that here, only with richcmp_by_eq_and_lt
.
comment:4 Changed 4 years ago by
 Description modified (diff)
comment:5 Changed 4 years ago by
Question: do you assume a total order? I am asking because you currently implement x <= y
as x == y
or x < y
. But you could also implement it as not x > y
. This way, you only need one method call for every comparison.
comment:6 Changed 4 years ago by
Answering my own question... the example shows that the order is partial.
comment:7 Changed 4 years ago by
 Dependencies changed from #23103 to #23102
comment:8 followup: ↓ 9 Changed 4 years ago by
 Branch set to u/jdemeyer/move_richcmp_by_eq_and_lt_to_structure_richcmp_pyx
comment:9 in reply to: ↑ 8 ; followup: ↓ 10 Changed 4 years ago by
 Commit set to 98fd0ccb74ec626a0c4f8d07af1d37e8d2841a56
Replying to jdemeyer:
if equal_types: other_eq = getattr(other, eq_attr) if other_eq(self): return rich_to_bool(op, 0)
What is the reason for using eq_attr
of other
and never the eq_attr
attribute of self
?
New commits:
256bf19  Move richcmp stuff to new file richcmp.pyx

b591b78  Support __richcmp__ in Python classes

98fd0cc  Move richcmp_by_eq_and_lt to structure/richcmp.pyx

comment:10 in reply to: ↑ 9 ; followups: ↓ 11 ↓ 14 Changed 4 years ago by
Replying to dkrenn:
What is the reason for using
eq_attr
ofother
and never theeq_attr
attribute ofself
?
It's only for efficiency. If we already know other_eq
, it's more efficient to use it instead. Python's implementation of rich comparisons assumes that a == b
is the same as b == a
, so it shouldn't hurt.
comment:11 in reply to: ↑ 10 Changed 4 years ago by
Replying to jdemeyer:
Replying to dkrenn:
What is the reason for using
eq_attr
ofother
and never theeq_attr
attribute ofself
?It's only for efficiency. If we already know
other_eq
, it's more efficient to use it instead. Python's implementation of rich comparisons assumes thata == b
is the same asb == a
, so it shouldn't hurt.
Ok.
comment:12 Changed 4 years ago by
 Status changed from new to needs_review
comment:13 followup: ↓ 15 Changed 4 years ago by
 I think the outer parentheses are not needed in
+ cdef bint equal_types = (type(self) is type(other))
 Is there a reason for using
op_LT
andPy_LT
?
 Docstring:
+  ``other``  arbitrary object. If it does have ``eq_attr`` and + ``lt_attr`` methods, those are used for the comparison. Otherwise, + the comparison is undefined.
those
vs. these
. I am not native English speaking, but shoudn't it be the latter?
Apart from these comments; the code looks fine. Let's see what the patchbot says.
comment:14 in reply to: ↑ 10 ; followup: ↓ 16 Changed 4 years ago by
Replying to jdemeyer:
Replying to dkrenn:
What is the reason for using
eq_attr
ofother
and never theeq_attr
attribute ofself
?It's only for efficiency. If we already know
other_eq
, it's more efficient to use it instead. Python's implementation of rich comparisons assumes thata == b
is the same asb == a
, so it shouldn't hurt.
I somehow find it strange that the comparision a == b
is performed by b
and not a
...
comment:15 in reply to: ↑ 13 ; followup: ↓ 18 Changed 4 years ago by
Replying to dkrenn:
 I think the outer parentheses are not needed in
+ cdef bint equal_types = (type(self) is type(other))
They are certainly not needed. It's just a style issue: when assigning the result of a comparisonlike operator, I find it clearer with the parentheses.
comment:16 in reply to: ↑ 14 ; followup: ↓ 19 Changed 4 years ago by
Replying to dkrenn:
I somehow find it strange that the comparision
a == b
is performed byb
and nota
...
It might be strange but it's not wrong. From PEP 207:
4 The reflexivity rules *are* assumed by Python. Thus, the interpreter may swap y>x with x<y, y>=x with x<=y, and may swap the arguments of x==y and x!=y.
comment:17 Changed 4 years ago by
 Status changed from needs_review to needs_work
comment:18 in reply to: ↑ 15 Changed 4 years ago by
Replying to jdemeyer:
Replying to dkrenn:
 I think the outer parentheses are not needed in
+ cdef bint equal_types = (type(self) is type(other))They are certainly not needed. It's just a style issue: when assigning the result of a comparisonlike operator, I find it clearer with the parentheses.
Ok, clearer is always better ;)
comment:19 in reply to: ↑ 16 Changed 4 years ago by
Replying to jdemeyer:
Replying to dkrenn:
I somehow find it strange that the comparision
a == b
is performed byb
and nota
...It might be strange but it's not wrong. From PEP 207:
4 The reflexivity rules *are* assumed by Python. Thus, the interpreter may swap y>x with x<y, y>=x with x<=y, and may swap the arguments of x==y and x!=y.
Ok, so let's keep it.
comment:20 Changed 4 years ago by
 Commit changed from 98fd0ccb74ec626a0c4f8d07af1d37e8d2841a56 to e732456a71ad73bbd82bbc9fe9fa8c663d8e43cf
Branch pushed to git repo; I updated commit sha1. New commits:
e732456  Use Py_EQ instead of op_EQ in Cython code

comment:21 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:22 Changed 4 years ago by
 Commit changed from e732456a71ad73bbd82bbc9fe9fa8c663d8e43cf to 9db7dccf0be5f0cd93071541556dac147e9cdfeb
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
49b6f76  Support __richcmp__ in Python classes

d343d9d  Implement RealSet comparisons using __richcmp__

3aac3cc  Typo

86e9fcf  Install slot wrappers for rich comparison methods

4d1021b  Further fixes and tests for richcmp_method

604f423  Move richcmp_by_eq_and_lt to structure/richcmp.pyx

9db7dcc  Use Py_EQ instead of op_EQ in Cython code

comment:23 Changed 4 years ago by
 Reviewers set to Daniel Krenn
 Status changed from needs_review to needs_work
Patchbot found failing tests in richcmp.pyx
comment:24 Changed 4 years ago by
Right, this got broken by the recent changes in #23102.
comment:25 followup: ↓ 27 Changed 4 years ago by
 Commit changed from 9db7dccf0be5f0cd93071541556dac147e9cdfeb to aa858075bb8dadd839ae5c744b8e81fb82e7f1c3
Branch pushed to git repo; I updated commit sha1. New commits:
aa85807  Fix richcmp_by_eq_and_lt example

comment:26 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:27 in reply to: ↑ 25 Changed 4 years ago by
comment:28 Changed 4 years ago by
 Status changed from needs_review to positive_review
The previous patchbot report only showed failures in richcmp.pyx
. I fixed those doctests and did not change any code, so it should be fine. Thanks for the review!
comment:29 Changed 4 years ago by
 Branch changed from u/jdemeyer/move_richcmp_by_eq_and_lt_to_structure_richcmp_pyx to aa858075bb8dadd839ae5c744b8e81fb82e7f1c3
 Resolution set to fixed
 Status changed from positive_review to closed
Replying to jdemeyer:
+1 :)