Fix broken np.allclose, add np.isclose #8857

lericson · 2023-03-29T10:56:39Z

It was previously impossible to use np.allclose as the default arguments rtol, atol, and equal_nan had types float and bool, whereas the code was testing for the Numba types Float and Boolean.

Also there was no np.isclose, the element-wise equivalent of np.allclose.

It is also debatable whether allclose should be an array method; it is not in numpy. I have not made isclose a method.

stuartarchibald · 2023-03-29T11:34:34Z

@lericson Many thanks for the patch. Please could you open an issue containing a minimal working reproducer for the bug(s) this is fixing and also please could appropriate tests be added for the proposed changes/bug fixes? Thanks again.

xref: #6286 which implements similar?

lericson · 2023-03-29T12:45:39Z

Here is a reproduction…

lericson@strixy:~$ ~/.pyenv/versions/3.10.6/bin/python3 -m venv tmp_env
lericson@strixy:~$ cd tmp_env
lericson@strixy:~/tmp_env$ ./bin/pip install numba
…
Successfully installed llvmlite-0.39.1 numba-0.56.4 numpy-1.23.5
lericson@strixy:~/tmp_env$ ./bin/python3
Python 3.10.6 (main, Aug 22 2022, 12:13:43) [GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.

Scalar-scalar case

>>> from numba import njit
>>> @njit
... def f(a, b):
...   return np.allclose(a, b)
...
>>> import numpy as np
>>> f(0.0, 1.0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/lericson/tmp_env/lib/python3.10/site-packages/numba/core/dispatcher.py", line 468, in _compile_for_args
    error_rewrite(e, 'typing')
  File "/home/lericson/tmp_env/lib/python3.10/site-packages/numba/core/dispatcher.py", line 409, in error_rewrite
    raise e.with_traceback(None)
numba.core.errors.TypingError: Failed in nopython mode pipeline (step: nopython frontend)
No implementation of function Function(<function allclose at 0x7f5bba179c60>) found for signature:

 >>> allclose(float64, float64)

There are 2 candidate implementations:
  - Of which 2 did not match due to:
  Overload in function 'np_allclose': File: numba/np/arraymath.py: Line 894.
    With argument(s): '(float64, float64)':
   Rejected as the implementation raised a specific error:
     TypeError: The third argument "rtol" must be a floating point
  raised from /home/lericson/tmp_env/lib/python3.10/site-packages/numba/np/arraymath.py:905

During: resolving callee type: Function(<function allclose at 0x7f5bba179c60>)
During: typing of call at <stdin> (3)


File "<stdin>", line 3:
<source missing, REPL/exec in use?>

Array-array case

>>> f(np.array([0.0]), np.array([1.0]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/lericson/tmp_env/lib/python3.10/site-packages/numba/core/dispatcher.py", line 468, in _compile_for_args
    error_rewrite(e, 'typing')
  File "/home/lericson/tmp_env/lib/python3.10/site-packages/numba/core/dispatcher.py", line 409, in error_rewrite
    raise e.with_traceback(None)
numba.core.errors.TypingError: Failed in nopython mode pipeline (step: nopython frontend)
No implementation of function Function(<function allclose at 0x7f5bba179c60>) found for signature:

 >>> allclose(array(float64, 1d, C), array(float64, 1d, C))

There are 2 candidate implementations:
    - Of which 2 did not match due to:
    Overload in function 'np_allclose': File: numba/np/arraymath.py: Line 894.
      With argument(s): '(array(float64, 1d, C), array(float64, 1d, C))':
     Rejected as the implementation raised a specific error:
       TypeError: The third argument "rtol" must be a floating point
  raised from /home/lericson/tmp_env/lib/python3.10/site-packages/numba/np/arraymath.py:905

During: resolving callee type: Function(<function allclose at 0x7f5bba179c60>)
During: typing of call at <stdin> (3)


File "<stdin>", line 3:
<source missing, REPL/exec in use?>

>>>

lericson · 2023-03-29T12:46:53Z

Note that the PR you xref'd seems to reimplement allclose also, maybe the PR is out of date.

Numba has np.allclose already, it just doesn't work at all.

stuartarchibald · 2023-03-29T13:25:21Z

Having just updated the release notes for the upcoming 0.57 release, I noticed that np.isclose is already implemented via #7067.

stuartarchibald · 2023-03-29T13:36:07Z

@lericson thanks for the reproducer. #8860 tracks.

guilhermeleobas · 2023-03-29T20:34:22Z

numba/np/arraymath.py

+@overload(np.isclose)
+def np_isclose(a, b, rtol=1e-05, atol=1e-08, equal_nan=False):


This file already contains an implementation of np.isclose in line 1191. Could you update the existing implementation? Or remove it in favor of this one?

lericson · 2023-03-30T07:35:32Z

@guilhermeleobas I think the existing implementation is just as good, let's ignore this PR since the issue seems resolved. As for #8860, it is also likely fixed with the next release. Thanks for a great project!

lericson · 2023-03-30T07:44:00Z

Actually on closer inspection, the code in master should be rewritten to reuse its _isclose_item between allclose and isclose. It may actually be preferable to use my code instead, as I use np.nditer, which doesn't accidentally densify 0-stride arrays. Let me demonstrate:

>>> import numpy as np
>>> a = np.arange(10)[:, None]
>>> b = np.arange(10)[None, :]
>>> ab, ba = np.broadcast_arrays(a, b)

The current implementation uses ndarray.reshape(-1), which sometimes copies memory:

>>> ab.shape
(10, 10)
>>> ab.strides
(8, 0)
>>> ab_flat = ab.reshape(-1)
>>> ab_flat.shape
(100,)
>>> ab_flat.strides
(8,)
>>> ab.base.nbytes
80
>>> ab_flat.base.nbytes
800

guilhermeleobas · 2023-03-30T13:47:47Z

It may actually be preferable to use my code instead, as I use np.nditer

Any chance you could replace the existing implementation by yours?

lericson · 2023-03-31T10:48:19Z

Certainly, just give me a little time- LudvigOn 30 Mar 2023, at 15:48, Guilherme Leobas ***@***.***> wrote: It may actually be preferable to use my code instead, as I use np.nditer Any chance you could replace the existing implementation by yours? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you modified the open/close state.Message ID: ***@***.***>

lericson · 2023-04-03T07:17:43Z

I suggest deprecating arr1.allclose(arr2), it's not in numpy and the existing isclose is not an array method either.

It was previously impossible to use `np.allclose` as the default arguments `rtol`, `atol`, and ´equal_nan` had types `float` and `bool`, whereas the code was testing for the Numba types `Float` and `Boolean`. Also replaced `np.isclose`, the element-wise equivalent of `np.allclose`. It is also debatable whether `allclose` should be an array method; it is not in numpy.

lericson · 2023-04-03T07:32:25Z

numba/np/arraymath.py

+                cv[()] = _isclose_scalars(av.item(), bv.item(), rtol=rtol,
+                                          atol=atol, equal_nan=equal_nan)
+
+            return c_c


By the way, wouldn't this variant work for all array-scalar combinations?

In fact I think this works for scalar-scalar also. At least in non-numba Numpy.

lericson · 2023-04-03T07:47:28Z

I tested it and this version uses way less code, and does exactly the same thing.

guilhermeleobas

Hi @lericson, there's currently an error on CI regarding using np.broadcast_arrays with arrays with different dtype. This is currently not supported in Numba as it is not possible to infer the output list dtype.

guilhermeleobas · 2023-04-03T15:02:38Z

numba/np/arraymath.py

+@overload(np.isclose)
+def np_isclose(a, b, rtol=1e-05, atol=1e-08, equal_nan=False):
+    def impl(a, b, rtol=1e-05, atol=1e-08, equal_nan=False):
+        ab, ba = np.broadcast_arrays(a, b)


rather than calling np.broadcast_arrays, compute the final shape with np.broadcast_shapes and call np.broadcast_to in each array individually. This might solve the current CI issue.

Interesting. I have changed the code. Pushing in a moment.

Hold on, why can't broadcast_arrays just return a Tuple(ArrayType1, ArrayType2)?

I would also note that np.nditer doesn't work quite the same as the true numpy version; my use of it here seems to fail. I have to manually broadcast the arrays.

Hold on, why can't broadcast_arrays just return a Tuple(ArrayType1, ArrayType2)?

We follow NumPy spec where np.broadcast_arrays returns a list.

guilhermeleobas

Hi @lericson, thanks for working on these changes.

Current changes are almost 2x slower (np.allclose) than previous code:

$ ./runtests.py -f -v numba.tests.test_np_functions -k allclose
test_allclose (numba.tests.test_np_functions.TestNPFunctions) ... ok
test_allclose_exception (numba.tests.test_np_functions.TestNPFunctions) ... ok
test_ip_allclose_numpy (numba.tests.test_np_functions.TestNPFunctions) ... ok
test_ip_not_allclose_numpy (numba.tests.test_np_functions.TestNPFunctions) ... ok

----------------------------------------------------------------------
Ran 4 tests in 3.802s

$ time ./runtests.py -f -v numba.tests.test_np_functions -k isclose
test_isclose (numba.tests.test_np_functions.TestNPFunctions) ... /Users/guilhermeleobas/git/numba/numba/tests/test_np_functions.py:135: NumbaExperimentalFeatureWarning: Use of isinstance() detected. This is an experimental feature.
  return np.isclose(a, b, rtol, atol, equal_nan)
/Users/guilhermeleobas/git/numba/numba/tests/test_np_functions.py:135: NumbaExperimentalFeatureWarning: Use of isinstance() detected. This is an experimental feature.
  return np.isclose(a, b, rtol, atol, equal_nan)
ok

----------------------------------------------------------------------
Ran 1 test in 4.077s

$ ./runtests.py -f -v numba.tests.test_np_functions -k allclose
test_allclose (numba.tests.test_np_functions.TestNPFunctions) ... /Users/guilhermeleobas/git/numba/numba/tests/test_np_functions.py:343: NumbaExperimentalFeatureWarning: Use of isinstance() detected. This is an experimental feature.
  return np.allclose(a, b, rtol, atol, equal_nan)
/Users/guilhermeleobas/git/numba/numba/tests/test_np_functions.py:343: NumbaExperimentalFeatureWarning: Use of isinstance() detected. This is an experimental feature.
  return np.allclose(a, b, rtol, atol, equal_nan)
ok
test_allclose_exception (numba.tests.test_np_functions.TestNPFunctions) ... ok
test_ip_allclose_numpy (numba.tests.test_np_functions.TestNPFunctions) ... /Users/guilhermeleobas/git/numba/numba/tests/test_np_functions.py:343: NumbaExperimentalFeatureWarning: Use of isinstance() detected. This is an experimental feature.
  return np.allclose(a, b, rtol, atol, equal_nan)
ok
test_ip_not_allclose_numpy (numba.tests.test_np_functions.TestNPFunctions) ... ok

----------------------------------------------------------------------
Ran 4 tests in 7.379s

$ ./runtests.py -f -v numba.tests.test_np_functions -k isclose
test_isclose (numba.tests.test_np_functions.TestNPFunctions) ... /Users/guilhermeleobas/git/numba/numba/tests/test_np_functions.py:135: NumbaExperimentalFeatureWarning: Use of isinstance() detected. This is an experimental feature.
  return np.isclose(a, b, rtol, atol, equal_nan)
/Users/guilhermeleobas/git/numba/numba/tests/test_np_functions.py:135: NumbaExperimentalFeatureWarning: Use of isinstance() detected. This is an experimental feature.
  return np.isclose(a, b, rtol, atol, equal_nan)
/Users/guilhermeleobas/git/numba/numba/tests/test_np_functions.py:135: NumbaExperimentalFeatureWarning: Use of isinstance() detected. This is an experimental feature.
  return np.isclose(a, b, rtol, atol, equal_nan)
ok

----------------------------------------------------------------------
Ran 1 test in 5.415s

It's not necessary to create and broadcast an array for scalar items, as it can be an expensive operation in terms of both compilation and execution. Could you investigate this a bit further?

guilhermeleobas · 2023-04-04T15:11:52Z

numba/np/arraymath.py

-    if not type_can_asarray(a):
-        raise TypeError('The first argument "a" must be array-like')
-
-    if not type_can_asarray(b):
-        raise TypeError('The second argument "b" must be array-like')
-
-    if not isinstance(rtol, types.Float):
-        raise TypeError('The third argument "rtol" must be a '
-                        'floating point')
-
-    if not isinstance(atol, types.Float):
-        raise TypingError('The fourth argument "atol" must be a '
-                          'floating point')
-
-    if not isinstance(equal_nan, types.Boolean):
-        raise TypeError('The fifth argument "equal_nan" must be a '
-                        'boolean')


When np.allclose is used with unsupported types, these exceptions provide a useful error message. Could we keep them?

guilhermeleobas · 2023-04-04T15:13:29Z

numba/np/arraymath.py

-    if not (type_can_asarray(a) and type_can_asarray(b)):
-        raise TypingError("Inputs for `np.isclose` must be array-like.")


Same thing here. Having the exception has its upsides.

guilhermeleobas · 2023-04-04T15:14:33Z

@lericson could you add a couple of tests for the case you reported in your first comment?

It was previously impossible to use np.allclose as the default arguments rtol, atol, and equal_nan had types float and bool, whereas the code was testing for the Numba types Float and Boolean.

lericson · 2023-04-05T08:12:34Z

Good luck!

lericson requested review from sklam and stuartarchibald as code owners March 29, 2023 10:56

stuartarchibald added the 2 - In Progress label Mar 29, 2023

lericson force-pushed the patch-1 branch 2 times, most recently from 93e8f77 to 7ca086f Compare March 29, 2023 12:38

guilhermeleobas reviewed Mar 29, 2023

View reviewed changes

lericson closed this Mar 30, 2023

lericson reopened this Mar 30, 2023

guilhermeleobas added the 4 - Waiting on author Waiting for author to respond to review label Mar 30, 2023

lericson added 2 commits April 3, 2023 09:18

Simplify _isclose_scalars logic

634b69d

lericson force-pushed the patch-1 branch from 7ca086f to 634b69d Compare April 3, 2023 07:19

lericson commented Apr 3, 2023

View reviewed changes

lericson force-pushed the patch-1 branch from 28a6334 to e2abeab Compare April 3, 2023 08:22

guilhermeleobas requested changes Apr 3, 2023

View reviewed changes

Reduce code in isclose/allclose implementation

0ca4791

lericson force-pushed the patch-1 branch from e2abeab to 0ca4791 Compare April 4, 2023 07:49

More careful in broadcasting in allclose/isclose

96f7d7c

guilhermeleobas requested changes Apr 4, 2023

View reviewed changes

lericson closed this Apr 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken np.allclose, add np.isclose #8857

Fix broken np.allclose, add np.isclose #8857

lericson commented Mar 29, 2023 •

edited

stuartarchibald commented Mar 29, 2023

lericson commented Mar 29, 2023

lericson commented Mar 29, 2023

stuartarchibald commented Mar 29, 2023

stuartarchibald commented Mar 29, 2023

guilhermeleobas Mar 29, 2023

lericson commented Mar 30, 2023

lericson commented Mar 30, 2023 •

edited

guilhermeleobas commented Mar 30, 2023

lericson commented Mar 31, 2023 via email

lericson commented Apr 3, 2023

lericson Apr 3, 2023

lericson Apr 3, 2023

lericson commented Apr 3, 2023

guilhermeleobas left a comment

guilhermeleobas Apr 3, 2023 •

edited

lericson Apr 4, 2023

lericson Apr 4, 2023

lericson Apr 4, 2023

guilhermeleobas Apr 4, 2023

guilhermeleobas left a comment

guilhermeleobas Apr 4, 2023

guilhermeleobas Apr 4, 2023

guilhermeleobas commented Apr 4, 2023

lericson commented Apr 5, 2023

		@overload(np.isclose)
		def np_isclose(a, b, rtol=1e-05, atol=1e-08, equal_nan=False):

		if not (type_can_asarray(a) and type_can_asarray(b)):
		raise TypingError("Inputs for `np.isclose` must be array-like.")

Fix broken np.allclose, add np.isclose #8857

Fix broken np.allclose, add np.isclose #8857

Conversation

lericson commented Mar 29, 2023 • edited

stuartarchibald commented Mar 29, 2023

lericson commented Mar 29, 2023

lericson commented Mar 29, 2023

stuartarchibald commented Mar 29, 2023

stuartarchibald commented Mar 29, 2023

Choose a reason for hiding this comment

lericson commented Mar 30, 2023

lericson commented Mar 30, 2023 • edited

guilhermeleobas commented Mar 30, 2023

lericson commented Mar 31, 2023 via email

lericson commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lericson commented Apr 3, 2023

guilhermeleobas left a comment

Choose a reason for hiding this comment

guilhermeleobas Apr 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermeleobas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermeleobas commented Apr 4, 2023

lericson commented Apr 5, 2023

lericson commented Mar 29, 2023 •

edited

lericson commented Mar 30, 2023 •

edited

guilhermeleobas Apr 3, 2023 •

edited