Opened 5 years ago
Closed 5 years ago
#23109 closed enhancement (fixed)
Move richcmp_by_eq_and_lt to structure/richcmp.pyx
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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 5 years ago by
comment:2 Changed 5 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 5 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 5 years ago by
- Description modified (diff)
comment:5 Changed 5 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 5 years ago by
Answering my own question... the example shows that the order is partial.
comment:7 Changed 5 years ago by
- Dependencies changed from #23103 to #23102
comment:8 follow-up: ↓ 9 Changed 5 years ago by
- Branch set to u/jdemeyer/move_richcmp_by_eq_and_lt_to_structure_richcmp_pyx
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 5 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 ; follow-ups: ↓ 11 ↓ 14 Changed 5 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 5 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 5 years ago by
- Status changed from new to needs_review
comment:13 follow-up: ↓ 15 Changed 5 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 ; follow-up: ↓ 16 Changed 5 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 ; follow-up: ↓ 18 Changed 5 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 comparison-like operator, I find it clearer with the parentheses.
comment:16 in reply to: ↑ 14 ; follow-up: ↓ 19 Changed 5 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 5 years ago by
- Status changed from needs_review to needs_work
comment:18 in reply to: ↑ 15 Changed 5 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 comparison-like operator, I find it clearer with the parentheses.
Ok, clearer is always better ;)
comment:19 in reply to: ↑ 16 Changed 5 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 5 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 5 years ago by
- Status changed from needs_work to needs_review
comment:22 Changed 5 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 5 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 5 years ago by
Right, this got broken by the recent changes in #23102.
comment:25 follow-up: ↓ 27 Changed 5 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 5 years ago by
- Status changed from needs_work to needs_review
comment:27 in reply to: ↑ 25 Changed 5 years ago by
comment:28 Changed 5 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 5 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 :)