Richcomp (magic methods) does not correctly implement >= compared to 0.19

[ChristopherRabotin]

Bug Description

Is there a way to disable the behavior introduced in #3203? I had (knowingly) implemented the __richcomp__ function (as shown below) and now the new method in 0.20 does not seem to implement it in the same way.

Custom code for 0.19

// ...
#[cfg(feature = "python")]
    fn __richcmp__(&self, other: Self, op: CompareOp) -> bool {
        match op {
            CompareOp::Lt => *self < other,
            CompareOp::Le => *self <= other,
            CompareOp::Eq => *self == other,
            CompareOp::Ne => *self != other,
            CompareOp::Gt => *self > other,
            CompareOp::Ge => *self >= other,
        }
    }
// ...

Proof:

In [1]: from hifitime import *

In [2]: Unit.Second * 0.0 == Duration("0 ns")
Out[2]: True

In [3]: Duration("0 ns") <= Unit.Second * 1.0
Out[3]: True

In [4]: Duration("0 ns") < Unit.Second * 1.0
Out[4]: True

In [5]: Unit.Second * 1.0 > Duration("0 ns")
Out[5]: True

In [6]: Unit.Second * 1.0 >= Duration("0 ns")
Out[6]: True

In [7]: Unit.Second * 1.0 >= Duration("-10 ns")

Automatic code in 0.20

https://github.com/nyx-space/hifitime/actions/runs/6898765377/job/18769311402?pr=259

Steps to Reproduce

1. Update to pyo3 0.20.0 from 0.19.0
2. Remove the custom __richcomp__ method
3. Run tests: https://github.com/nyx-space/hifitime/actions/runs/6898765377/job/18769311402?pr=259

Backtrace

def test_duration_eq():
        """
        Checks that Duration comparisons work
        """
    
        assert Unit.Second * 0.0 == Duration("0 ns")
>       assert Unit.Second * 1.0 >= Duration("0 ns")
E       TypeError: '>=' not supported between instances of 'builtins.Duration' and 'builtins.Duration'

tests/python/test_epoch.py:54: TypeError
=========================== short test summary info ============================
FAILED tests/python/test_epoch.py::test_duration_eq - TypeError: '>=' not supported between instances of 'builtins.Duration' and 'builtins.Duration'
========================= 1 failed, 3 passed in 0.12s ==========================

Your operating system and version

Windows and macOS

Your Python version (python --version)

3.11

Your Rust version (rustc --version)

1.74

Your PyO3 version

0.20.0

How did you install python? Did you use a virtualenv?

Using the maturin action on Github

Additional Info

No response

[davidhewitt]

@ChristopherRabotin in your linked PR you delete __richcmp__ and so all that remains is __eq__. I don't see implementations of ordering operators.

If you want ordering you should implement at least __lt__ and __le__. Because your types are symmetric you don't strictly need __gt__ and __ge__, the interpreter will fallback on __le__, and __lt__ by reversing the operands. For best performance you should implement all four of __lt__, __le__, __gt__ and __ge__.

[ChristopherRabotin]

Understood, thanks. I had misunderstood the issue as saying that all rich comparisons had been implemented by default.

…

On Fri, Nov 17, 2023, 06:23 David Hewitt ***@***.***> wrote: @ChristopherRabotin <https://github.com/ChristopherRabotin> in your linked PR you delete __richcmp__ and so all that remains is __eq__. I don't see implementations of ordering operators. If you want ordering you should implement at least __lt__ and __le__. Because your types are symmetric you don't strictly need __gt__ and __ge__, the interpreter will fallback on __le__, and __lt__ by reversing the operands. For best performance you should implement all four of __lt__, __le__, __gt__ and __ge__. — Reply to this email directly, view it on GitHub <#3581 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABEZV2D4OZHZ4XPIZXASNXTYE5QOJAVCNFSM6AAAAAA7PDJXVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJWGQZDENJRGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>