pytest v6.2.0 pytest.approx failing tests when pytest v6.1.2 passes

[matthewfeickert]

(This is a cross-post of scikit-hep/pyhf#1219)

• a detailed description of the bug or problem you are having

On 2020-12-12 scikit-hep/pyhf@f824afe ran on CI on pyhf's master and succeeded. Later that day pytest v6.2.0 was released and the nightly scheduled CI failed on

_______________________ test_optim_with_value[jax-mu=1] ________________________

backend = (<pyhf.tensor.jax_backend.jax_backend object at 0x7f6bf92def50>, None)
source = {'bindata': {'bkg': [100.0, 150.0], 'bkgsys_dn': [98, 100], 'bkgsys_up': [102, 190], 'data': [120.0, 180.0], ...}, 'binning': [2, -0.5, 1.5]}
spec = {'channels': [{'name': 'singlechannel', 'samples': [{'data': [30.0, 95.0], 'modifiers': [{...}], 'name': 'signal'}, {'data': [100.0, 150.0], 'modifiers': [{...}], 'name': 'background'}]}]}
mu = 1.0

    @pytest.mark.parametrize('mu', [1.0], ids=['mu=1'])
    def test_optim_with_value(backend, source, spec, mu):
        pdf = pyhf.Model(spec)
        data = source['bindata']['data'] + pdf.config.auxdata
    
        init_pars = pdf.config.suggested_init()
        par_bounds = pdf.config.suggested_bounds()
    
        optim = pyhf.optimizer
    
        result = optim.minimize(pyhf.infer.mle.twice_nll, data, pdf, init_pars, par_bounds)
        assert pyhf.tensorlib.tolist(result)
    
        result, fitted_val = optim.minimize(
            pyhf.infer.mle.twice_nll,
            data,
            pdf,
            init_pars,
            par_bounds,
            fixed_vals=[(pdf.config.poi_index, mu)],
            return_fitted_val=True,
        )
        assert pyhf.tensorlib.tolist(result)
        assert pyhf.tensorlib.shape(fitted_val) == ()
>       assert pytest.approx(17.52954975, rel=1e-5) == fitted_val
E       assert 17.52954975 ± 1.8e-04 == DeviceArray(17.52954975, dtype=float64)
E        +  where 17.52954975 ± 1.8e-04 = <function approx at 0x7f6cc1747f80>(17.52954975, rel=1e-05)
E        +    where <function approx at 0x7f6cc1747f80> = pytest.approx

tests/test_optim.py:383: AssertionError

Diffing the installed libraries between the two (in f824afe_install.txt and failing_install.txt) shows that the relevant change is pytest

$ diff f824afe_install.txt failing_install.txt 
33a34
> importlib-metadata     3.1.1
83c84
< py                     1.9.0
---
> py                     1.10.0
96c97
< pytest                 6.1.2
---
> pytest                 6.2.0
143a145
> zipp                   3.4.0

This is confirmed as if

--- a/setup.py
+++ b/setup.py
@@ -29,7 +29,7 @@
         + extras_require['contrib']
         + extras_require['shellcomplete']
         + [
-            'pytest~=6.0',
+            'pytest~=6.1.0',
             'pytest-cov>=2.5.1',
             'pytest-mock',
             'pytest-benchmark[histogram]',

the CI installs v6.1.2 and passes.

The only mention of pytest.approxin the v6.2.0 release notes is

7710: Use strict equality comparison for non-numeric types in pytest.approx instead of
raising TypeError.

This was the undocumented behavior before 3.7, but is now officially a supported feature.

• output of pip list from the virtual environment you are using

• f824afe_install.txt

• failing_install.txt

• pytest and operating system versions

• Failing: pytest v6.2.0 on ubuntu-latest and macos-latest on GitHub Actions

• Passing: pytest v6.1.2 on ubuntu-latest and macos-latest on GitHub Actions

• minimal example if possible

I don't have a trivial minimal failing example at time of reporting (I can work on that if desired), but the CI tests that caught this are detailed in scikit-hep/pyhf#1219 and an example of v6.1.2 passing can be see in scikit-hep/pyhf#1220.

cc @lukasheinrich @kratsg

[matthewfeickert]

Hm. Okay, reading PR #7710

This makes x == approx(y) equivalent to x == y if x or y are not instances of decimal.Decimal or numbers.Complex, rather than raising a TypeError.

I see that this is because the comparisons of n-d array objects to floats is the issue and that this can be fixed on my side with

assert pytest.approx(17.52954975, rel=1e-5) == pyhf.tensorlib.tolist(fitted_val)

Though this change in behavior comes as a bit of surprise. I think that this Issue can get closed, but would the dev team consider a separate Issue that might expand the possible comparisons? Though maybe it is better to just have people be better about the comparisons here and really compare floats to floats.

[nicoddemus]

Hi @matthewfeickert!

Thanks for the report.

It seems the bit of code responsible for the change is:

https://github.com/pytest-dev/pytest/pull/7710/files#diff-1118e2a1281a92eca6051881271a30ab8ada17354c867a086f68d9f2f7ea2807R248-R255

Previously it seems everything would work with DeviceArray because it implements the necessary methods for approx-comparison like __abs__, __sub__, and __float__.

Perhaps we should fix this and instead of using isinstance(..., (Complex, Decimal)), check for those methods instead?

cc @jvansanten

[jvansanten]

I think the underlying issue (which I admit I did expose with #7710) is that pytest relies on isinstance(obj, np.ndarray) to detect arrays. This causes objects that are implicitly convertible to ndarray, but do not not use numpy's memory allocation, to fall through to the scalar case, where they will happen to give the expected result if they have only one element. Here's a minimal example that reproduces the essence of the issue:

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

        class SecretNumpyArray:
            def __init__(self, value):
                self.__array_interface__ = value.__array_interface__

        expected = 1
        assert approx(expected) == np.asarray(SecretNumpyArray(np.array([expected+1e-6])))
        assert approx(expected) == SecretNumpyArray(np.array([expected+1e-6]))

Even though both right-hand sides are equivalent from numpy's perspective, the first assert will pass and the second will fail. It would be more consistent to modify python_api._is_numpy_array() to detect objects that are implicitly convertible to ndarray and send them down the same path that direct subclasses of ndarray take. I will try to think of a way to do that without introducing too many unnecessary conversions.

[nicoddemus]

Thanks @jvansanten!

Indeed that would be another improvement, I did however take a different approach in #8136, which will also handle non-numpy objects. Feel free to comment over there. 👍