bugfix in ApproxNumpy initialisation, use keywords for arguments to fix

[abrammer]

With respect to #3695 , flip the arguments in the initialization and also use keywords so it's more obvious and less likely to happen in future.

Can add tests along the lines of the code in #3695 if desired but it seems like an obvious typo fix

[coveralls]

Coverage increased (+0.05%) to 92.655% when pulling 535fd1f on abrammer:approx_numpy_tolerance_bugfix into 9720c33 on pytest-dev:master.

[asottile]

Let's add a quick test since the current code doesn't actually ensure ApproxNumpy works

[abrammer]

You think add a test to check tolerances, or just make the current tests such that abs & rel tolerances aren't equivalent. I can push a commit either way this eve.

[asottile]

Anything that fails before your change and passes afterwards. Either approach is fine though the "different params" one is what I'd probably go for.

[abrammer]

Something else is broken here. I typo'd the last rel argument, so it's 10% instead of 1%.
Fixing the typo means that it fails though, which doesn't make sense as 99 is obviously within 1% of 100.

[abrammer]

Justification for f0db64a:

currently pytest will calculate the tolerances against either value if it's a numpy array.
The below will pass a test in 3.6.4.
np.array(1e7) == pytest.approx(1, abs=1)

With the earlier commits, the below will still pass.
np.array(1e7) == pytest.approx(1, rel=1)

Unless I'm missing something the approx should be 1±1 therefore it should be wrong.
I have stripped out the repeat ApproxNumpy() initilsiation at

pytest/src/_pytest/python_api.py

Line 214 in b7419bd

return ApproxNumpy(actual, self.abs, self.rel, self.nan_ok) == self.expected

as we already have our self.expected and self.tolerance. so I'm not sure why there would be another Approx initialization. This may not elegantly handle nans but should just fail.

[nicoddemus]

LGTM, thanks a lot @abrammer!

[tadeu]

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 ;)

[kalekundert]

The extra ApproxNumpy initialization was there to properly handle corner-cases involving nan and inf. In particular, the nan_ok option will be ignored without it. That said, looking back at this code now, the extra ApproxNumpy initialization was also broken because it switched the "actual" and "expected" values, which would cause relative tolerances to be calculated incorrectly.

I think the best way to write this line would be like this:

        if _is_numpy_array(actual):
            return all(a == self for a in actual)

Thanks for the PR and the tests (clearly this code needs more tests)!

[nicoddemus]

Thanks @kalekundert for the suggestion! I implemented it myself, I think we can merge this for now, it has been waiting here long enough. I encourage others to open PRs later to improve the tests if needed! 👍

[abrammer]

I have changes and tests coming, away from my computer for a couple days so will update with respect to the @kalekundert suggestion and tests on Sunday.

@nicoddemus that will fail on a 0d(?) array. E.g. np.array(100). If you can hold off until Sunday I'll get a commit in with chnages to cover that and tests for nans as well.

[nicoddemus]

@abrammer sure thing, let's wait for you to get around to this then! Thanks a lot for following up with this. 👍

[tadeu]

Just brainstorming, but it would be less errorprone to maintain if all corner cases are tested (it's hard to remember all corner cases when maintaining the code):

• 0-d, 1-d, 2-d
• common values, nan, inf
• check for rel only (equality and inequality); cases where abs would fail
• check for abs only (equality and inequality); cases where rel would fail
• no numpy array, numpy array on left-side, numpy array on right-side, numpy array on both sides
• pytest.approx on left-side, pytest.approx on right side
• relative tolerance that would fail if sides are switched

Perhaps an approach with parametrize would be helpful.
I could work on this if you think it's useful (would it be another issue?)

[abrammer]

@tadeu I think the tests I put in cover most of those cases. Pulled logic from earlier tests, though maybe a decorator method could be better?

I was going to suggest np.at_least1d(actual) with respect to bf127a6, might be a little more memory efficient but probably doesn't matter for tests.

[kalekundert]

Looks good to me. I think one of flat, flatten(), or ravel() is needed to handle comparisons with multidimensional arrays, and my understanding is that flat just makes an iterator and doesn't copy anything. But I haven't really thought about it that carefully.

[abrammer]

Yeah you're right. Flatten copies, flat just iterates. Nice.

[tadeu]

@abrammer, awesome improvement on the tests ;)