Skip to content
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

Approx with inequalities #2003

Closed
adler-j opened this Issue Oct 17, 2016 · 24 comments

Comments

Projects
None yet
7 participants
@adler-j
Copy link

adler-j commented Oct 17, 2016

The new approx function is great, but does not seem to work with inequalities, instead failing silently.

I.e.

def test_aprox_inequal():
    assert pytest.approx(0.0) <= 5.0

gives

    def test_aprox_inequal():
>       assert pytest.approx(0.0) <= 5.0
E       assert 0.0 ± 1.0e-12 <= 5.0
E        +  where 0.0 ± 1.0e-12 = <class '_pytest.python.approx'>(0.0)
E        +    where <class '_pytest.python.approx'> = pytest.approx

It would be great if inequality testing was also supported with approx, and if not then this should give a better error message.

@rogalski

This comment has been minimized.

Copy link

rogalski commented Nov 20, 2016

I started some drafts on it.

Just wondering, what would be a best way to handle sequences? I've considered:

  1. Nodewise comparison and ValueError if item count between expected and result differs?
  2. Unconditional ValueError: cannot compare iterables for [inequality]?

Inequality is likely a misleading name, but I don't know a general english word for >, >=, <, <= operations.

Decision made here also should be complaint with whatever will be decided about #1994.

@adler-j

This comment has been minimized.

Copy link
Author

adler-j commented Nov 20, 2016

I dont think we want a situation where these give wildly different results:

assert arr <= approx(0.5)
assert arr <= 0.5

Perhaps the best is to simply defer the evaluation to the corresponding operator of the underlying type?

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Nov 20, 2016

I think unconditionally raising a ValueError is a good solution... I don't see much value in supporting assert pytest.approx(0.0) <= 5.0, it is essentially the same as assert 0.0 <= 5.0.

@adler-j

This comment has been minimized.

Copy link
Author

adler-j commented Nov 20, 2016

I don't see much value in supporting assert pytest.approx(0.0) <= 5.0, it is essentially the same as assert 0.0 <= 5.0.

The same logic could be applied to the very existence of pytest.approx when applied to ==, no?

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Nov 20, 2016

The same logic could be applied to the very existence of pytest.approx when applied to ==, no?

IMHO no, because for equality there's more logic involved, the equivalent code would be (roughly) assert round(x, 2) == 0.1 or something like that, although the current approx code takes care of some edge cases.

Or is there a use case for assert pytest.approx(x) <= 0.1 that I'm not seeing?

@rogalski

This comment has been minimized.

Copy link

rogalski commented Nov 20, 2016

@nicoddemus approx(x) <= 0.1 where x = 0.1 + 1e-10. At least that's how I understood reported issue.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Nov 20, 2016

Hmm I see, thanks. Well if it is simple to implement I guess it wouldn't hurt supporting it then.

@adler-j

This comment has been minimized.

Copy link
Author

adler-j commented Nov 20, 2016

@nicoddemus approx(x) <= 0.1 where x = 0.1 + 1e-10. At least that's how I understood reported issue.

This is exactly what I was aiming at.

@timofurrer

This comment has been minimized.

Copy link

timofurrer commented Jun 5, 2017

I'd really like to see this to be supported thus I had a quick look into it.

I encountered some issue with the implementation though:

assert approx(0.1) > 0.1 + 1e-10  # calls approx(0.1).__gt__(0.1 + 1e-10)
assert 0.1 + 1e-10 > approx(0.1)  # calls approx(0.1).__lt__(0.1 + 1e-10)

In the second example one could expect approx(0.1).__le__(0.1 + 1e-10) to be called.

The Python docs write the following about it:

There are no swapped-argument versions of these methods (to be used when the left argument does not support the operation but the right argument does); rather, lt() and gt() are each other’s reflection, le() and ge() are each other’s reflection, and eq() and ne() are their own reflection. If the operands are of different types, and right operand’s type is a direct or indirect subclass of the left operand’s type, the reflected method of the right operand has priority, otherwise the left operand’s method has priority. Virtual subclassing is not considered.
-> See https://docs.python.org/3/reference/datamodel.html#object.__ge__)

I don't see another way but to restrict that approx is only allowed being always on the same side of the inequality operator.

This is not very intuitive at all and will eventually lead to confusion - IMHO, even if the documentation about it would be sufficient to understand it.

Any thoughts about it?

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 6, 2017

I don't see another way but to restrict that approx is only allowed being always on the same side of the inequality operator.
This is not very intuitive at all and will eventually lead to confusion - IMHO, even if the documentation about it would be sufficient to understand it.

I agree that users might get tripped by this, but given that the language itself doesn't allow for us to implement the operator when approx is on the right side, I don't see an alternative. I would say documenting it is the best we can do in this case.

Perhaps @kalekundert (who added approx in #1441) can chime in as well?

@timofurrer

This comment has been minimized.

Copy link

timofurrer commented Jun 6, 2017

I agree that users might get tripped by this, but given that the language itself doesn't allow for us to implement the operator when approx is on the right side, I don't see an alternative. I would say documenting it is the best we can do in this case.

Yeah, I agree. I'll finish the work I've done so far and I'll submit a PR for review in the next few days, if that's okay.

@kalekundert

This comment has been minimized.

Copy link
Contributor

kalekundert commented Jun 6, 2017

Hmmm, I don't understand why one would expect assert 0.1 + 1e-10 > approx(0.1) to call approx(0.1).__le__(0.1 + 1e-10). The assertion should be False, because 0.1 + 1e-10 is close enough to 0.1 to be considered equal to it (therefore it's not greater than it). A less-than test from the perspective of approx(0.1) would also return False, but a less-than-or-equal test would return True. But I have to admit that I don't fully understand the use-case for this feature, so maybe I'm misinterpreting something.

@adler-j

This comment has been minimized.

Copy link
Author

adler-j commented Jun 7, 2017

My expectation from approx would be that it should always be "on the generous side" towards True.

As an example of where I would use approx for stuff like this, consider this:

for i in range(100):
    assert 1 + 2 ** (-i) > 1

which (at i=53), raises an AssertionError since 1 + 2 ** (-53) == 1. Now, if i added approx (to either side, really) I'd expect it to never raise an AssertionError.

@kalekundert

This comment has been minimized.

Copy link
Contributor

kalekundert commented Jun 7, 2017

I think I see where you're coming from now (approx.__eq__ is "generous", so approx.__gt__ should be too). I don't agree with this, though. I think it's crucial for the behavior of == and > to be self-consistent. That is, if two numbers are equal, one can't also be greater than or less than the other.

I think >= and <= are the appropriate operators for getting generous behavior. I'm not opposed to adding all four inequality operators to the approx class. These operators do still strike me as unnecessary (because there aren't any subtleties in checking if one float is less-than or greater-than another), but I can see the benefit of having approx objects act more like regular numbers (i.e. having a better abstraction).

@adler-j

This comment has been minimized.

Copy link
Author

adler-j commented Jun 8, 2017

Another option which is perfectly valid would be to simply use strict checking for < and > and then allow the generous behavior for >= and <=. With that said, following truly self-consistent behavior is bound for issues. I mean, who would truly expect this to happen:

>>> pytest.approx(1.0 + 1e-5) > 1.0
False

just because we wanted to be generous with <=

>>> pytest.approx(1.0 + 1e-5) <= 1.0
True
@kalekundert

This comment has been minimized.

Copy link
Contributor

kalekundert commented Jun 8, 2017

I would be ok with me if we just implemented >= and <= and had > and < raise NotImplementedError or something.

@adler-j

This comment has been minimized.

Copy link
Author

adler-j commented Jun 9, 2017

I agree, simply raising NotImplementedError is vastly superior to confusing behavior.

@maiksensi

This comment has been minimized.

Copy link
Contributor

maiksensi commented Jul 12, 2017

I followed the discussion briefly and while at EuroPython I thought why not give it a shot as this is marked easy :).
@kalekundert would you mind checking master...maiksensi:bug/issue-2003-approx whether this fits the scope (raise a NotImplementedError) outlined by you?
I would then add some docs describing the reasoning behind the new behavior and of course docstrings and submit a PR.

@kalekundert

This comment has been minimized.

Copy link
Contributor

kalekundert commented Jul 13, 2017

Looks good to me. But you should pull from the features branch again before making a pull request. I did some pretty significant refactoring to the approx class in #2492, and the code was moved from python.py to python_api.py around the same time.

@maiksensi

This comment has been minimized.

Copy link
Contributor

maiksensi commented Jul 13, 2017

Okay good to know. I will do that. I wasn't sure, as this was tagged as "bug" and the contribution guidelines say that one should branch from master (https://github.com/pytest-dev/pytest/blob/master/CONTRIBUTING.rst#preparing-pull-requests-on-github) then.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jul 13, 2017

after taking a look again, its quite clear that my original labeling was a mistake

@maiksensi

This comment has been minimized.

Copy link
Contributor

maiksensi commented Jul 13, 2017

Thanks @RonnyPfannschmidt for clarifying this. I will submit a PR in the next few days then.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 25, 2017

Guys could you take a look at the latest version of #2576? In that form we are raising a TypeError for all operators other than == and !=, as I was under the impression we wanted to be consistent and avoid surprises. If I'm wrong in my understanding please leave a comment there!

nicoddemus added a commit that referenced this issue Jul 30, 2017

Merge pull request #2576 from maiksensi/feat/raise-not-implemented-fo…
…r-lt-gt-in-approx

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

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 30, 2017

Fixed by #2576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.