Add support for pytest.approx comparisons between array and scalar

[tadeu]

fixes #3312

[coveralls]

Coverage decreased (-0.005%) to 92.909% when pulling a754f00 on tadeu:approx-array-scalar into cbb2c55 on pytest-dev:features.

[nicoddemus]

Awesome, thanks @tadeu!

[nicoddemus]

@kalekundert would you like to take a look as well?

[kalekundert]

It took me a while to realize that in the ApproxNumpy class, both self.expected and actual can now be scalars. So I think it'd be good to mention that (and to describe how self.expected can end up as a scalar, because it's pretty subtle) in the docstring.

It's a little unfortunate that ApproxNumpy gets so much more complicated having to handle 3 combinations of different expected/actual types. You could possibly simplify things by not allowing self.expected to be a scalar, and handling the case where the expected value is a scalar and the actual value is an array in ApproxScalar without invoking ApproxNumpy, e.g.:

all(
    actual[i] == self._approx_scalar(self.expected)
    for i in np.ndindex(actual.shape)
)

[kalekundert]

Can you update this comment to note that self.expected and actual can both be either arrays or scalars?

[tadeu]

Sure, I'm changing it to

        # For both `actual` and `self.expected`, they can independently be
        # either a `numpy.array` or a scalar (but both can't be scalar,
        # in this case an `ApproxScalar` is used).
        # They are treated in `__eq__` before being passed to
        # `ApproxBase.__eq__`, which is the only method that calls this one.

[kalekundert]

If self.expected can be a scalar, __repr__ should probably be able to handle that. Although it's more of a philosophical concern than a practical one, since __repr__ shouldn't be called with a scalar self.expected as the code currently stands. Still, who knows what could happen in the future.

[tadeu]

You're right, I'll to improve this
(done)

[tadeu]

all(
    actual[i] == self._approx_scalar(self.expected)
    for i in np.ndindex(actual.shape)
)

Hmm, this could be done, but I think it would actually complicate things a liitle bit more, because the part to display a nice message like

>       assert actual == approx(expected, rel=5e-7, abs=0)
E       assert array([1.00001, 0.99999]) == 1.0 ± 5.0e-07
E        +  where 1.0 ± 5.0e-07 = approx(1.0, rel=5e-07, abs=0)

would also need to be reimplemented (get which value is different, etc.).

[tadeu]

@kalekundert all done, would you like to take another look?

[kalekundert]

Either way, ApproxScalar.__repr__ is going to be called to get the nice message to display. That won't be affected by anything that happens in ApproxScalar.__eq__. In other words, the code responsible for making a nice message (if the expected value is a scalar) is ApproxScalar.__repr__, and it doesn't even know about the actual value (or whether or not it's an array). The ApproxScalar.__eq__ method just needs to return true or false, correctly taking into account the tolerances.

The more I think about it, the more I think ApproxNumpy.expected should always be an array. Getting philosophical again, the Approx* classes are basically wrappers around an expected value, and it doesn't really make sense for there to be two ways to wrap a scalar (especially since one would only work if you then subsequently compared it to an array).

Anyways, I'm worried that I'm losing sight of the fact that the code is totally fine. :-) So I'll leave it to your judgment. If you think it'd be better (and worth the effort) to require ApproxNumpy.expected to be an array, go ahead and do it. Otherwise let me know and I'll merge.

[tadeu]

Hi @kalekundert, I agree with you, I've changed the code so that self.expected in ApproxNumpy is always an array now, the code is simpler ;)

[kalekundert]

Awesome, thanks for the PR!

[nicoddemus]

Great, thanks @tadeu and @kalekundert!