New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Behaviour of richcmp function need some additional explaining #1848

Closed
SleepProgger opened this Issue Aug 29, 2017 · 16 comments

Comments

Projects
None yet
5 participants
@SleepProgger

SleepProgger commented Aug 29, 2017

At the moment the behavior of the __richcmp__ function isn't really clear, especially in regards to different types and unsupported comparisons..

According to some testing i did:

  • If only one of components is a cdefed class AND has a __richcmp__ function that function is used irregardless of the component order. If both components have a __richcmp__ function the lefts components function is used.
  • If the comparison is not == or != and the left component doesn't support richcmp the op is flipped (< to > and so on) and the richcmp function of the right side is used.

Is this correct so far ?

Also the arithmetic functions support returning NotImplemented to signal that they do not support this operation on the given datatypes and the other side should be tried.
Is there something similar for comparisons ?

I have a Vector class for example, and only want to implement the == and != operations. How should that be handled ?
Simply raise a NotImplementedError would kind of work, but prevents the other side from handling that operation if it is able to.

@robertwb

This comment has been minimized.

Show comment
Hide comment
@robertwb

robertwb Oct 13, 2017

Contributor

Now that __eq__, __le__, etc. are supported, they should be preferred to __richcmp__. If the semantics are not the same as in Python, file a bug. Possibly we should update our docs (PRs welcome).

Contributor

robertwb commented Oct 13, 2017

Now that __eq__, __le__, etc. are supported, they should be preferred to __richcmp__. If the semantics are not the same as in Python, file a bug. Possibly we should update our docs (PRs welcome).

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Oct 18, 2017

Contributor

At the moment the behavior of the __richcmp__ function isn't really clear, especially in regards to different types and unsupported comparisons..

It's exactly the same as the __eq__ methods and friends in plain Python, except that there is only 1 special method (__richcmp__) where the op argument denotes the operation. That's really the only difference.

If you want details of how rich comparisons work, I would refer to PEP 207: https://www.python.org/dev/peps/pep-0207/

If the comparison is not == or != and the left component doesn't support richcmp the op is flipped (< to > and so on) and the richcmp function of the right side is used.

This is documented in PEP 207

Is there something similar for comparisons ?

Again as documented in PEP 207, just return NotImplemented.

Contributor

jdemeyer commented Oct 18, 2017

At the moment the behavior of the __richcmp__ function isn't really clear, especially in regards to different types and unsupported comparisons..

It's exactly the same as the __eq__ methods and friends in plain Python, except that there is only 1 special method (__richcmp__) where the op argument denotes the operation. That's really the only difference.

If you want details of how rich comparisons work, I would refer to PEP 207: https://www.python.org/dev/peps/pep-0207/

If the comparison is not == or != and the left component doesn't support richcmp the op is flipped (< to > and so on) and the richcmp function of the right side is used.

This is documented in PEP 207

Is there something similar for comparisons ?

Again as documented in PEP 207, just return NotImplemented.

@rwst

This comment has been minimized.

Show comment
Hide comment
@rwst

rwst Nov 23, 2017

See https://trac.sagemath.org/ticket/24255 for an example why this is a Cython bug that already hits users.

rwst commented Nov 23, 2017

See https://trac.sagemath.org/ticket/24255 for an example why this is a Cython bug that already hits users.

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 25, 2017

Contributor

See https://trac.sagemath.org/ticket/24255 for an example why this is a Cython bug that already hits users.

What is the Cython bug? Can you be more clear please...

Contributor

jdemeyer commented Nov 25, 2017

See https://trac.sagemath.org/ticket/24255 for an example why this is a Cython bug that already hits users.

What is the Cython bug? Can you be more clear please...

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 25, 2017

Contributor

If the semantics are not the same as in Python, file a bug.

My guess is that most people don't know the Python semantics and blame everything on Cython.

Contributor

jdemeyer commented Nov 25, 2017

If the semantics are not the same as in Python, file a bug.

My guess is that most people don't know the Python semantics and blame everything on Cython.

@scoder scoder closed this in 2fc2cba Nov 25, 2017

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Nov 25, 2017

Contributor

I've marked __richcmp__() as deprecated in the docs. That should solve this problem. It feels useless to repeat major parts of PEP 207 in the Cython docs if we can just refer to the normal Python docs of the comparison methods.

Contributor

scoder commented Nov 25, 2017

I've marked __richcmp__() as deprecated in the docs. That should solve this problem. It feels useless to repeat major parts of PEP 207 in the Cython docs if we can just refer to the normal Python docs of the comparison methods.

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 25, 2017

Contributor

I've marked __richcmp__() as deprecated in the docs.

Why? I think the __richcmp__ interface is much better than the 6 methods __eq__ and friends. In SageMath, I even made a decorator to allow __richcmp__ in pure Python classes.

I can understand that some people might prefer __eq__, but deprecating __richcmp__ sounds like a step too far.

Contributor

jdemeyer commented Nov 25, 2017

I've marked __richcmp__() as deprecated in the docs.

Why? I think the __richcmp__ interface is much better than the 6 methods __eq__ and friends. In SageMath, I even made a decorator to allow __richcmp__ in pure Python classes.

I can understand that some people might prefer __eq__, but deprecating __richcmp__ sounds like a step too far.

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Nov 25, 2017

Contributor

I didn't say that we'd remove it (and it's certainly not the right time to think about that). I only wrote that people should use the normal Python way instead.

I think the __richcmp__ interface is much better than the 6 methods __eq__ and friends

Could you elaborate? Having two interfaces towards the same functionality should be justified. What advantage does __richcmp__() have over the One Python Way to do it?

Contributor

scoder commented Nov 25, 2017

I didn't say that we'd remove it (and it's certainly not the right time to think about that). I only wrote that people should use the normal Python way instead.

I think the __richcmp__ interface is much better than the 6 methods __eq__ and friends

Could you elaborate? Having two interfaces towards the same functionality should be justified. What advantage does __richcmp__() have over the One Python Way to do it?

@rwst

This comment has been minimized.

Show comment
Hide comment
@rwst

rwst Nov 25, 2017

What is the Cython bug? Can you be more clear please...

I was confused by Python2 old-style vs new-style classes.

rwst commented Nov 25, 2017

What is the Cython bug? Can you be more clear please...

I was confused by Python2 old-style vs new-style classes.

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 25, 2017

Contributor

What advantage does __richcmp__() have over the One Python Way to do it?

Briefly, __richcmp__ leads to less code duplication. Often, the six methods __eq__ and friends will have a very similar implementation. So you end up writing mostly the same code six times.

Also, given that __richcmp__ maps directly to the C API, it's probably faster too.

Contributor

jdemeyer commented Nov 25, 2017

What advantage does __richcmp__() have over the One Python Way to do it?

Briefly, __richcmp__ leads to less code duplication. Often, the six methods __eq__ and friends will have a very similar implementation. So you end up writing mostly the same code six times.

Also, given that __richcmp__ maps directly to the C API, it's probably faster too.

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Nov 25, 2017

Contributor

I wouldn't bring in the performance argument here. Either the implementation is so short that the overhead really matters, in which case there probably won't be any substantial code duplication and everything would just get inlined by the C compiler and result in exactly the same performance. Or the implementation is complex and duplicated, in which case the overhead probably doesn't matter and it won't make a difference either.

I do, however, accept the argument that in complex cases, the implementation might really be so non-trivial and similar that a joined method reduces the overall code duplication and complexity.

Contributor

scoder commented Nov 25, 2017

I wouldn't bring in the performance argument here. Either the implementation is so short that the overhead really matters, in which case there probably won't be any substantial code duplication and everything would just get inlined by the C compiler and result in exactly the same performance. Or the implementation is complex and duplicated, in which case the overhead probably doesn't matter and it won't make a difference either.

I do, however, accept the argument that in complex cases, the implementation might really be so non-trivial and similar that a joined method reduces the overall code duplication and complexity.

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 25, 2017

Contributor

I do, however, accept the argument that in complex cases, the implementation might really be so non-trivial and similar that a joined method reduces the overall code duplication and complexity.

The code duplication is not limited to "complex cases". It is quite unlikely that __lt__ and __le__ and __gt__ and __ge__ are implemented very differently. Typically, they behave mostly the same. So you end up with code duplication.

Contributor

jdemeyer commented Nov 25, 2017

I do, however, accept the argument that in complex cases, the implementation might really be so non-trivial and similar that a joined method reduces the overall code duplication and complexity.

The code duplication is not limited to "complex cases". It is quite unlikely that __lt__ and __le__ and __gt__ and __ge__ are implemented very differently. Typically, they behave mostly the same. So you end up with code duplication.

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 25, 2017

Contributor

In SageMath, I even made a decorator to allow richcmp in pure Python classes.

Let me elaborate a bit on this: SageMath deals with mathematics, so comparisons are very important. SageMath did this mostly by implementing __cmp__ and using cmp(). This obviously needed to be changed for Python 3 (in fact, I guess that this is probably the biggest Py3 porting issue of SageMath).

Often, __cmp__ methods look like this:

def __cmp__(self, other):
    x = (self.attr1, self.attr2)
    y = (other.attr1, other.attr2)
    return cmp(x, y)

Now it's easy to port this to __richcmp__:

def __richcmp__(self, other, int op):
    x = (self.attr1, self.attr2)
    y = (other.attr1, other.attr2)
    return PyObject_RichCompare(x, y, op)

But if you want to do this in plain Python, you have to write the same thing six times.

This __richcmp__ method calling PyObject_RichCompare turned out to be so convenient that we wanted to use the same thing in plain Python too. So I made a decorator @richcmp_method and a function richcmp() which allows you to write in plain Python

@richcmp_method
class Foo(object):
    def __richcmp__(self, other, op):
        x = (self.attr1, self.attr2)
        y = (other.attr1, other.attr2)
        return richcmp(x, y, op)
Contributor

jdemeyer commented Nov 25, 2017

In SageMath, I even made a decorator to allow richcmp in pure Python classes.

Let me elaborate a bit on this: SageMath deals with mathematics, so comparisons are very important. SageMath did this mostly by implementing __cmp__ and using cmp(). This obviously needed to be changed for Python 3 (in fact, I guess that this is probably the biggest Py3 porting issue of SageMath).

Often, __cmp__ methods look like this:

def __cmp__(self, other):
    x = (self.attr1, self.attr2)
    y = (other.attr1, other.attr2)
    return cmp(x, y)

Now it's easy to port this to __richcmp__:

def __richcmp__(self, other, int op):
    x = (self.attr1, self.attr2)
    y = (other.attr1, other.attr2)
    return PyObject_RichCompare(x, y, op)

But if you want to do this in plain Python, you have to write the same thing six times.

This __richcmp__ method calling PyObject_RichCompare turned out to be so convenient that we wanted to use the same thing in plain Python too. So I made a decorator @richcmp_method and a function richcmp() which allows you to write in plain Python

@richcmp_method
class Foo(object):
    def __richcmp__(self, other, op):
        x = (self.attr1, self.attr2)
        y = (other.attr1, other.attr2)
        return richcmp(x, y, op)
@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Nov 25, 2017

Contributor
Contributor

scoder commented Nov 25, 2017

@jdemeyer

This comment has been minimized.

Show comment
Hide comment
@jdemeyer

jdemeyer Nov 26, 2017

Contributor

Most use cases will however only involve equality comparisons

In such a case, you would still want __eq__ and __ne__. But that can be solved by

def __ne__(self, other):
    return not (self == other)

Which makes me think... perhaps Cython should offer (a better) variant of functools.total_order (which is IMHO pretty badly implemented).

Contributor

jdemeyer commented Nov 26, 2017

Most use cases will however only involve equality comparisons

In such a case, you would still want __eq__ and __ne__. But that can be solved by

def __ne__(self, other):
    return not (self == other)

Which makes me think... perhaps Cython should offer (a better) variant of functools.total_order (which is IMHO pretty badly implemented).

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Nov 26, 2017

Contributor

In such a case, you would still want __eq__ and __ne__

https://github.com/cython/cython/blob/0.27.3/Cython/Compiler/ModuleNode.py#L1842

perhaps Cython should offer (a better) variant of functools.total_order

Sounds good to me. But I think we're getting out of scope for this (closed) ticket...

Contributor

scoder commented Nov 26, 2017

In such a case, you would still want __eq__ and __ne__

https://github.com/cython/cython/blob/0.27.3/Cython/Compiler/ModuleNode.py#L1842

perhaps Cython should offer (a better) variant of functools.total_order

Sounds good to me. But I think we're getting out of scope for this (closed) ticket...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment