#2003 Change behavior of approx.py to only support __eq__ comparison

[maiksensi]

This PR changes the current behavior of approx to raise NotImplementedError for all comparisons but __eq__ as discussed in #2003

Note: I thought about adding a warn that this is currently not supported like:

warnings.warn("Currently not supported")
raise NotImplementedError

But we found #2452 mentioning that pytest.config.warn should be deprecated. Also, I was not sure how to use this in approx, as it is not a fixture and the config object is not available out of the box. Can we just use the warnings module there? Or is mentioning this in the docs enough?

[coveralls]

Coverage decreased (-0.01%) to 91.887% when pulling d8eface on maiksensi:feat/raise-not-implemented-for-lt-gt-in-approx into 1485a3a on pytest-dev:features.

[nicoddemus]

First of all thanks a ton for the PR! 👍 🎆

According to the documentation, we should return the NotImplemented singleton instead:

A rich comparison method may return the singleton NotImplemented if it does not implement the operation for a given pair of arguments.

I tried your PR locally and it givens this error:

____________________________________ test _____________________________________

    def test():
>       assert 1 > pytest.approx(1, rel=1e-6, abs=1e-12)

.tmp\test_original.py:4:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = 1 ± 1.0e-06, actual = 1

    def __lt__(self, actual):
>       raise NotImplementedError
E       NotImplementedError

_pytest\python_api.py:38: NotImplementedError
========================== 1 failed in 0.04 seconds ===========================

Changing to return NotImplemented gives a better error I think:

____________________________________ test _____________________________________

    def test():
>       assert 1 > pytest.approx(1, rel=1e-6, abs=1e-12)
E       TypeError: '>' not supported between instances of 'int' and 'ApproxScalar'

.tmp\test_original.py:4: TypeError
========================== 1 failed in 0.03 seconds ===========================

[maiksensi]

@nicoddemus thanks for having a look at it that fast. I should have read the whole part of the doc I guess :). I will submit another commit that returns a singleton. I guess I'll get to it the end of the weekend.

Did you think about my comment, about raising a user warning? I mentioned it in this PR. Does that make sense and do you have any tip in doing so?

But we found #2452 mentioning that pytest.config.warn should be deprecated. Also, I was not sure how to use this in approx, as it is not a fixture and the config object is not available out of the box. Can we just use the warnings module there? Or is mentioning this in the docs enough?

[nicoddemus]

Did you think about my comment, about raising a user warning?

I don't think a warning is necessary given that now it will fail with a TypeError. 😁

[maiksensi]

@nicoddemus I just had a brief look and it seems that just returning NotImplemented does not lead to a TypeError. Judging from the doc, the compiler will fallback to do the rich comparsion vise versa, instead of raising a TypeError. It seems this is not what was discussed in the #2003 issue. Maybe it makes sense to go back to the ticket first and discuss what we really want?

[nicoddemus]

Judging from the doc, the compiler will fallback to do the rich comparsion vise versa

Yes, but we have to return NotImplemented from all the rich-comparison functions (IIRC that's what I did). Did you change all raise NotImplementedError statements to return NotImplemented or just one of them?

[maiksensi]

So, I had some time to test this. I changed all (__lt__ and __gt__) to return NotImplemented. For python3 this is leading to the expected TypeError: TypeError: '>' not supported between instances of 'int' and 'ApproxScalar'. But Python2 seems to do it differently. It is not throwing a warning but instead returning True or False for < and >, respectively.

Python 2 also calls __cmp__ so I thought returning NotImplemented there would raise the error but it doesnt. Any suggestions? We could raise a TypeError in __cmp__ but thats not a good idea I guess :).

Python 2.7.12 (default, Nov 19 2016, 06:48:10) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from pytest import approx
>>> 1 > approx(1, rel=1e-6, abs=1e-12)
False
>>> 1 < approx(1, rel=1e-6, abs=1e-12)
True

      def __gt__(self, actual):
        return NotImplemented

    def __lt__(self, actual):
        return NotImplemented

    def __cmp__(self, actual):
        return NotImplemented

[nicoddemus]

@maiksensi that's weird, __cmp__ docs say:

Called by comparison operations if rich comparison (see above) is not defined.

I don't know how to proceed actually, perhaps we should look into why is Python calling __cmp__ even though we implement the rich-comparison operators.

Hmm just occurred to me, we shouldn't even implement __cmp__ at all. If you remove that, does it work?

[maiksensi]

If you remove that, does it work?

It doesn't change the behavior.

>>> a
1 +- 1.0e-06
>>> b
1
>>> type(b)
<type 'int'>
>>> b < a 
True
>>> b > a
False
>>> pdb.runeval(b<a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/pdb.py", line 1241, in runeval
    return Pdb().runeval(expression, globals, locals)
  File "/usr/lib/python2.7/bdb.py", line 416, in runeval
    expr = expr+'\n'
TypeError: unsupported operand type(s) for +: 'bool' and 'str'
>>> pdb.runeval(b>a)
> /home/maiksen/repos/pytest/_pytest/python_api.py(38)__lt__()
-> return NotImplemented

Is the interpreter maybe falling back to some sort of identity comparison?

[RonnyPfannschmidt]

@maiksensi you need to put " around the expressions, you pass in the result not a expression

[maiksensi]

@RonnyPfannschmidt ah thanks! Obviously, didn't know that. Will do and see how it goes.

[maiksensi]

So, giving it a shot the correct way:

>>> pdb.runeval("b>a")
> <string>(1)<module>()->True
(Pdb) s
--Call--
> /home/maiksen/repos/pytest/_pytest/python_api.py(37)__lt__()
-> def __lt__(self, actual):
(Pdb) s
> /home/maiksen/repos/pytest/_pytest/python_api.py(38)__lt__()
-> return NotImplemented
(Pdb) s
--Return--
> /home/maiksen/repos/pytest/_pytest/python_api.py(38)__lt__()->NotImplemented
-> return NotImplemented
(Pdb) s
--Return--
> <string>(1)<module>()->False
(Pdb) s
> /usr/lib/python2.7/bdb.py(422)runeval()
-> self.quitting = 1
(Pdb) s
False
>>> pdb.runeval("b<a")
> <string>(1)<module>()->False
(Pdb) s
--Call--
> /home/maiksen/repos/pytest/_pytest/python_api.py(34)__gt__()
-> def __gt__(self, actual):
(Pdb) s
> /home/maiksen/repos/pytest/_pytest/python_api.py(35)__gt__()
-> return NotImplemented
(Pdb) s
--Return--
> /home/maiksen/repos/pytest/_pytest/python_api.py(35)__gt__()->NotImplemented
-> return NotImplemented
(Pdb) s
--Return--
> <string>(1)<module>()->True
(Pdb) s
> /usr/lib/python2.7/bdb.py(422)runeval()
-> self.quitting = 1
(Pdb) s
True
>>> 

and with python 3.6 getting the expected TypeError

>>> pdb.runeval("b<a")
> <string>(1)<module>()->None
(Pdb) s
--Call--
> /home/maiksen/repos/pytest/_pytest/python_api.py(180)__gt__()
-> def __gt__(self, actual):
(Pdb) s
> /home/maiksen/repos/pytest/_pytest/python_api.py(181)__gt__()
-> return NotImplemented
(Pdb) s
--Return--
> /home/maiksen/repos/pytest/_pytest/python_api.py(181)__gt__()->NotImplemented
-> return NotImplemented
(Pdb) s
TypeError: '<' not supported between instances of 'int' and 'ApproxScalar'
> <string>(1)<module>()->None
(Pdb) s
--Return--
> <string>(1)<module>()->None
(Pdb) s
TypeError: '<' not supported between instances of 'int' and 'ApproxScalar'
...

[nicoddemus]

Unfortunately just defining the rich-comparison operators in Python 2 is not enough it seems.

I went ahead and raised a TypeError from __cmp__ in Python 2 and I think it is good enough:

# content of test_foo.py
import pytest

def test_aprox_inequal_le():
    assert pytest.approx(0.0) <= 0.0

def test_aprox_inequal_l():
    assert pytest.approx(0.0) < 0.0

def test_aprox_inequal_ge():
    assert pytest.approx(0.0) >= 0.0

def test_aprox_inequal_g():
    assert pytest.approx(0.0) > 0.0

def test_equal():
    assert pytest.approx(0.0) == 0.0

Python 2:

============================= test session starts =============================
platform win32 -- Python 2.7.13, pytest-3.1.4.dev153+g3eb4b16.d20170722, py-1.4.34, pluggy-0.4.0
rootdir: C:\pytest, inifile: tox.ini
plugins: ast-back-to-python-0.1.0, hypothesis-3.13.1
collected 5 items

.tmp\test_foo.py FFFF.

================================== FAILURES ===================================
____________________________ test_aprox_inequal_le ____________________________

    def test_aprox_inequal_le():
>       assert pytest.approx(0.0) <= 0.0
E       TypeError: Comparison operators other than == and != not supported by approx objects

.tmp\test_foo.py:5: TypeError
____________________________ test_aprox_inequal_l _____________________________

    def test_aprox_inequal_l():
>       assert pytest.approx(0.0) < 0.0
E       TypeError: Comparison operators other than == and != not supported by approx objects

.tmp\test_foo.py:8: TypeError
____________________________ test_aprox_inequal_ge ____________________________

    def test_aprox_inequal_ge():
>       assert pytest.approx(0.0) >= 0.0
E       TypeError: Comparison operators other than == and != not supported by approx objects

.tmp\test_foo.py:12: TypeError
____________________________ test_aprox_inequal_g _____________________________

    def test_aprox_inequal_g():
>       assert pytest.approx(0.0) > 0.0
E       TypeError: Comparison operators other than == and != not supported by approx objects

.tmp\test_foo.py:16: TypeError
===================== 4 failed, 1 passed in 0.06 seconds ======================

Python 3:

============================= test session starts =============================
platform win32 -- Python 3.6.0, pytest-3.1.4.dev153+g3eb4b16.d20170722, py-1.4.34, pluggy-0.4.0
rootdir: C:\pytest, inifile: tox.ini
plugins: xdist-1.17.0, hypothesis-3.13.1
collected 5 items

.tmp\test_foo.py FFFF.

================================== FAILURES ===================================
____________________________ test_aprox_inequal_le ____________________________

    def test_aprox_inequal_le():
>       assert pytest.approx(0.0) <= 0.0
E       TypeError: '<=' not supported between instances of 'ApproxScalar' and 'float'

.tmp\test_foo.py:5: TypeError
____________________________ test_aprox_inequal_l _____________________________

    def test_aprox_inequal_l():
>       assert pytest.approx(0.0) < 0.0
E       TypeError: '<' not supported between instances of 'ApproxScalar' and 'float'

.tmp\test_foo.py:8: TypeError
____________________________ test_aprox_inequal_ge ____________________________

    def test_aprox_inequal_ge():
>       assert pytest.approx(0.0) >= 0.0
E       TypeError: '>=' not supported between instances of 'ApproxScalar' and 'float'

.tmp\test_foo.py:12: TypeError
____________________________ test_aprox_inequal_g _____________________________

    def test_aprox_inequal_g():
>       assert pytest.approx(0.0) > 0.0
E       TypeError: '>' not supported between instances of 'ApproxScalar' and 'float'

.tmp\test_foo.py:16: TypeError
===================== 4 failed, 1 passed in 0.06 seconds ======================

The message is different because we can't know which operator triggered __cmp__ in Python 2, but I think that's OK.

Here's my patch (based on this branch):

diff --git a/_pytest/python_api.py b/_pytest/python_api.py
index d605add..ba6ea5a 100644
--- a/_pytest/python_api.py
+++ b/_pytest/python_api.py
@@ -31,15 +31,14 @@ class ApproxBase(object):

     __hash__ = None

-    def __gt__(self, actual):
-        raise NotImplementedError
-
-    def __lt__(self, actual):
-        raise NotImplementedError
-
     def __ne__(self, actual):
         return not (actual == self)

+    if sys.version_info.major == 2:
+        def __cmp__(self, other):
+            __tracebackhide__ = True
+            raise TypeError('Comparison operators other than == and != not supported by approx objects')
+
     def _approx_scalar(self, x):
         return ApproxScalar(x, rel=self.rel, abs=self.abs, nan_ok=self.nan_ok)

@@ -112,11 +111,10 @@ class ApproxNumpyBase(ApproxBase):
         return "approx({0!r})".format(list(
             self._approx_scalar(x) for x in self.expected))

-    def __gt__(self, actual):
-        raise NotImplementedError
-
-    def __lt__(self, actual):
-        raise NotImplementedError
+    if sys.version_info.major == 2:
+        def __cmp__(self, other):
+            __tracebackhide__ = True
+            raise TypeError('Comparison operators other than == and != not supported by approx objects')

     def __eq__(self, actual):
         import numpy as np

What do you guys think?

[maiksensi]

I would have proposed to manually raise a TypeError for Python 2, so I am fine with it. In particular I like that you implement __cmp__ as inner function, which sets it into context and thus makes it clear why it is needed. I would go ahead in the next days and shamelessly copy your code into this branch, sync with features and see if CI checks out.

[nicoddemus]

I think I still have this change locally at home. If you are OK with it, I can push it to your branch then.

[maiksensi]

Sure!

…

On Jul 25, 2017 5:18 PM, "Bruno Oliveira" ***@***.***> wrote: I think I still have this change locally at home. If you are OK with it, I can push it to your branch then. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2576 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGEJWuNbUXU7ocG_0_yxNAGyMwfjCFS_ks5sRgcxgaJpZM4OZA-z> .

[nicoddemus]

Applied the changes and rebased on features.

[coveralls]

Coverage decreased (-0.04%) to 91.88% when pulling a152cfe on maiksensi:feat/raise-not-implemented-for-lt-gt-in-approx into 309152d on pytest-dev:features.

[coveralls]

Coverage decreased (-0.04%) to 91.88% when pulling 80f4699 on maiksensi:feat/raise-not-implemented-for-lt-gt-in-approx into 309152d on pytest-dev:features.