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.

Sign up for GitHub

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

bugfix in ApproxNumpy initialisation, use keywords for arguments to fix #3696

Merged
merged 10 commits into from
Jul 30, 2018
Merged

bugfix in ApproxNumpy initialisation, use keywords for arguments to fix #3696

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7bff586
bugfix in ApproxNumpy initialisation, use keywords for arguments now
abrammer-climacell Jul 18, 2018
75db608
update changelog
abrammer Jul 18, 2018
514ca6f
add test wrt #3695 checking numpy array tolerance args
Jul 24, 2018
f0db64a
drop the duplicate approx call
Jul 25, 2018
2eb9301
Improve CHANGELOG
nicoddemus Jul 25, 2018
d0ba242
Implement change suggested by @kalekundert in PR
nicoddemus Jul 27, 2018
bf127a6
Need to iterate over the flattened array.
kalekundert Jul 27, 2018
330640e
update tests to check tolerance args and expecing nan in numpy arrays
Jul 30, 2018
762eaf4
update changelog to include the addition of tests
Jul 30, 2018
535fd1f
may as well include inf test while we're at it
Jul 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/3695.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix ApproxNumpy initialisation argument mixup. abs rel tolerances were previously flipped
5 changes: 4 additions & 1 deletion src/_pytest/python_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,10 @@ def __eq__(self, actual):
the pre-specified tolerance.
"""
if _is_numpy_array(actual):
return ApproxNumpy(actual, self.abs, self.rel, self.nan_ok) == self.expected
return (
ApproxNumpy(actual, rel=self.rel, abs=self.abs, nan_ok=self.nan_ok)
== self.expected
)

# Short-circuit exact equality.
if actual == self.expected:
Expand Down
14 changes: 14 additions & 0 deletions testing/python/approx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,20 @@ def test_numpy_array(self):
assert actual == approx(list(expected), rel=5e-7, abs=0)
assert actual != approx(list(expected), rel=5e-8, abs=0)

def test_numpy_tolerance_args(self):
"""
quick check that numpy rel/abs args are handled correctly
for comparison against an np.array
"""
np = pytest.importorskip("numpy")
expected = 100
actual = 99
assert actual != pytest.approx(expected, abs=0.1, rel=0)
assert np.array(actual) != pytest.approx(expected, abs=0.1, rel=0)

assert actual == pytest.approx(expected, abs=0, rel=0.01)
assert np.array(actual) == pytest.approx(expected, abs=0, rel=0.1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @abrammer!

What do you think about improving this test so that it is even more complete?
I'm thinking about adding these complementary checks also:

        assert 0.099 == pytest.approx(0.1, abs=0.01, rel=0)
        assert np.array(0.099) == pytest.approx(0.1, abs=0.01, rel=0)
        # ... other variants with np.array in other positions ...

        assert 0.099 != pytest.approx(0.1, abs=0, rel=0.005)
        assert np.array(0.099) != pytest.approx(0.1, abs=0, rel=0.005)
        # ... other variants with np.array in other positions ...

also, perhaps, let's add the equivalent for all tests, but with pytest.approx on the left-side. It would be a total of 32 asserts, but the more tested it is, the better ;)

def test_numpy_array_wrong_shape(self):
np = pytest.importorskip("numpy")

Expand Down