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: 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:

Status badges

Description (last modified by 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__.

Change History (29)

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

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

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

comment:4 Changed 4 years ago by jdemeyer

  • Description modified (diff)

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

comment:6 Changed 4 years ago by jdemeyer

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

comment:7 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23103 to #23102

comment:8 follow-up: Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/move_richcmp_by_eq_and_lt_to_structure_richcmp_pyx

comment:9 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by dkrenn

  • 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:

256bf19Move richcmp stuff to new file richcmp.pyx
b591b78Support __richcmp__ in Python classes
98fd0ccMove richcmp_by_eq_and_lt to structure/richcmp.pyx

comment:10 in reply to: ↑ 9 ; follow-ups: Changed 4 years ago by 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.

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

comment:12 Changed 4 years ago by jdemeyer

  • Status changed from new to needs_review

comment:13 follow-up: Changed 4 years ago by dkrenn

  1. I think the outer parentheses are not needed in
    +        cdef bint equal_types = (type(self) is type(other))
    
  1. Is there a reason for using op_LT and Py_LT?
  1. 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: Changed 4 years ago by 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...

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

comment:16 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by 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.  (Note: Python currently assumes
      that x==x is always true and x!=x is never true; this should not
      be assumed.)
Version 0, edited 4 years ago by jdemeyer (next)

comment:17 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

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

comment:20 Changed 4 years ago by git

  • Commit changed from 98fd0ccb74ec626a0c4f8d07af1d37e8d2841a56 to e732456a71ad73bbd82bbc9fe9fa8c663d8e43cf

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

e732456Use Py_EQ instead of op_EQ in Cython code

comment:21 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:22 Changed 4 years ago by git

  • Commit changed from e732456a71ad73bbd82bbc9fe9fa8c663d8e43cf to 9db7dccf0be5f0cd93071541556dac147e9cdfeb

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

49b6f76Support __richcmp__ in Python classes
d343d9dImplement RealSet comparisons using __richcmp__
3aac3ccTypo
86e9fcfInstall slot wrappers for rich comparison methods
4d1021bFurther fixes and tests for richcmp_method
604f423Move richcmp_by_eq_and_lt to structure/richcmp.pyx
9db7dccUse Py_EQ instead of op_EQ in Cython code

comment:23 Changed 4 years ago by dkrenn

  • 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 jdemeyer

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

comment:25 follow-up: Changed 4 years ago by git

  • Commit changed from 9db7dccf0be5f0cd93071541556dac147e9cdfeb to aa858075bb8dadd839ae5c744b8e81fb82e7f1c3

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

aa85807Fix richcmp_by_eq_and_lt example

comment:26 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:27 in reply to: ↑ 25 Changed 4 years ago by dkrenn

Replying to git:

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

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

comment:28 Changed 4 years ago by jdemeyer

  • 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 vbraun

  • 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
Note: See TracTickets for help on using tickets.