Move richcmp_by_eq_and_lt to structure/richcmp.pyx

[jdemeyer]

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__.

[dkrenn]

Replying to jdemeyer:

The function richcmp_by_eq_and_lt from src/sage/rings/asymptotic/misc.py should be moved to src/sage/structure/richcmp.pyx.

+1 :)

[jdemeyer]

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.

[jdemeyer]

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.

[jdemeyer]

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.

[jdemeyer]

Answering my own question... the example shows that the order is partial.

[dkrenn]

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

[jdemeyer]

Replying to dkrenn:

What is the reason for using eq_attr of other and never the eq_attr attribute of self?

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.

[dkrenn]

Replying to jdemeyer:

Replying to dkrenn:

What is the reason for using eq_attr of other and never the eq_attr attribute of self?

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.

Ok.

[dkrenn]

1. I think the outer parentheses are not needed in

   +        cdef bint equal_types = (type(self) is type(other))
   

2. Is there a reason for using op_LT and Py_LT?

3. 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.

[dkrenn]

Replying to jdemeyer:

Replying to dkrenn:

What is the reason for using eq_attr of other and never the eq_attr attribute of self?

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.

I somehow find it strange that the comparision a == b is performed by b and not a...

[jdemeyer]

Replying to dkrenn:

1. 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.

[jdemeyer]

Replying to dkrenn:

I somehow find it strange that the comparision a == b is performed by b and not a...

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.

[dkrenn]

Replying to jdemeyer:

Replying to dkrenn:

1. 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 ;)

[dkrenn]

Replying to jdemeyer:

Replying to dkrenn:

I somehow find it strange that the comparision a == b is performed by b and not a...

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.

[git]

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

​e732456

Use Py_EQ instead of op_EQ in Cython code

[git]

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

[dkrenn]

Patchbot found failing tests in richcmp.pyx

[jdemeyer]

Right, this got broken by the recent changes in #23102.

[git]

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

​aa85807

Fix richcmp_by_eq_and_lt example

[dkrenn]

Replying to git:

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

​aa85807

Fix richcmp_by_eq_and_lt example

Thank you for fixing this; LGTM. As soon as the patchbot confirms, this is a positive review from my side.

[jdemeyer]

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!