astropy.units.isclose broken in astropy 5.1

[maxnoe]

Description

astropy.units.isclose returns false for values within floating point precisions

Expected behavior

astropy.units.isclose returns true for values within floating point precisions

Actual behavior

astropy.units.isclose returns false for values within floating point precisions

Steps to Reproduce

In [1]: import astropy.units as u

In [2]: import numpy as np

In [3]: u.isclose(0 * u.m, np.finfo(np.float64).eps * u.m)
Out[3]: False

In [4]: u.isclose(0, np.finfo(np.float64).eps)
Out[4]: False

In [5]: np.isclose(0, np.finfo(np.float64).eps)
Out[5]: True.

System Details

Linux-5.10.109-1-MANJARO-x86_64-with-glibc2.10
Python 3.8.13 | packaged by conda-forge | (default, Mar 25 2022, 06:04:18) 
[GCC 10.3.0]
Numpy 1.22.3
pyerfa 2.0.0.1
astropy 5.1
Scipy 1.8.0
Matplotlib 3.5.1

[pllim]

Thanks for reporting this. But has this ever worked? The discrepancy is because the defaults are different. I went through the _unquantify_allclose_arguments logic and these are the numbers passed into numpy at the end.

actual = 0.0
desired = 2.220446049250313e-16
rtol = 1e-05
atol = 0.0

Meanwhile, this is the numpy default: Signature: np.isclose(a, b, rtol=1e-05, atol=1e-08, equal_nan=False)

So, you should do this instead when comparing zeroes:

>>> u.isclose(0 * u.m, np.finfo(np.float64).eps * u.m, atol=1e-8 * u.m)
True

I am not sure if the different default setting was by design, and outdated default (maybe numpy changed it upstream?), or a bug. If we decided to patch this behavior, should add a test with the use case stated above.

[maxnoe]

I got a broken test in the CI of ctapipe after it started using 5.1

[maxnoe]

https://github.com/cta-observatory/ctapipe/runs/6625449488?check_suite_focus=true

[pllim]

All this code that I mentioned has not been touched for at least 2 years. What version of astropy did you upgrade from? For now, explicitly setting atol for your test is backward compatible and will fix your pipeline failure.

[maxnoe]

The previous runs were using 5.0.4, strange

[pllim]

What was the value of back.x with astropy 5.0.x? Maybe this is a symptom of a different bug.

Fun fact: I came across this:

• numpy.isclose vs math.isclose numpy/numpy#10161

[maxnoe]

Mmh. I am confused now. I cannot reproduce locally. Test passes with 5.1 locally... So it has to be something else, sorry for the fuzz. Still confusing why it worked before.

[github-actions]

Hi humans 👋 - this issue was labeled as Close? approximately 6 hours ago. If you think this issue should not be closed, a maintainer should remove the Close? label - otherwise, I will close this issue in 7 days.

If you believe I commented on this issue incorrectly, please report this here

[mhvk]

@maxnoe - the reasoning behind the different default for atol is that for quantities with units it really is not obvious what the default should be. As in the discussion pointed to by @pllim, there is even a more general question why one would pick any atol - are 1e-20 and 1e-10 really close? It would seem to depend on what one is interested in!

Anyway, I'll close this since it doesn't seem there was a change in behaviour from the astropy side...