Deprecate redefinition of `np.testing.assert_allclose` #784

Dhruvanshu-Joshi · 2024-05-27T15:28:23Z

Description

Replaced all instances of assert_allclose with corresponding np.testing.assert_allclose.

Related Issue

Closes Remove reimplementation of np.testing.assert_allclose #551
Related to

Checklist

Checked that the pre-commit linting/style checks pass
Included tests that prove the fix is effective or that the new feature works
Added necessary documentation (docstrings and/or example notebooks)
If you are a pro: each commit corresponds to a relevant logical change

Type of change

codecov · 2024-06-03T07:12:14Z

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.96%. Comparing base (5c8afae) to head (87c6ee6).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #784      +/-   ##
==========================================
- Coverage   80.97%   80.96%   -0.02%     
==========================================
  Files         169      169              
  Lines       47015    47011       -4     
  Branches    11497    11497              
==========================================
- Hits        38072    38061      -11     
- Misses       6728     6729       +1     
- Partials     2215     2221       +6

Files	Coverage Δ
pytensor/tensor/math.py	`90.09% <ø> (-0.34%)`	⬇️
pytensor/tensor/type.py	`93.73% <33.33%> (-0.81%)`	⬇️

... and 3 files with indirect coverage changes

tests/unittest_tools.py

tests/scan/test_rewriting.py

ricardoV94

Asking whether we can remove one more helper from the non-test codebase

ricardoV94 · 2024-07-03T08:00:59Z

pytensor/tensor/type.py

+            atol_, rtol_ = pytensor.tensor.math._get_atol_rtol(a, b)
+            if rtol is not None:
+                rtol_ = rtol
+            if atol is not None:
+                atol_ = atol


Coverage says this is not being used, can we just remove?

ricardoV94 · 2024-07-03T08:01:24Z

tests/graph/test_compute_test_value.py

@@ -85,7 +85,15 @@ def test_variable_only(self):
        z = dot(x, y)
        assert hasattr(z.tag, "test_value")
        f = pytensor.function([x, y], z)
-        assert _allclose(f(x.tag.test_value, y.tag.test_value), z.tag.test_value)
+        atol_, rtol_ = _get_atol_rtol(


If this ends up only being used in the tests here, we can remove from math and include only in the test file?

Dhruvanshu-Joshi · 2024-07-15T14:51:33Z

So what is to be done here. Here's what is changing. So originally in pytensor.tensor.type, the function def values_eq_approx uses _allclose function from pytensor.tensor.math. Here if atol and rtol values are not provided, _allclose makes a call to _get_atol_rtol_values which calculates them. Now since we are removing the _allclose function altogether, I used the _get_atol_rtol_values directly in pytensor.tensor.type. And because _get_atol_rtol_values is used in pytensor.tensor.type, we cannot move it into unittest_tools.py in tests.

Dhruvanshu-Joshi · 2024-10-03T15:18:12Z

Hi @ricardoV94
Is there something left here? I don't understand why the coverage test is failing.

ricardoV94 · 2024-10-15T05:36:30Z

tests/tensor/rewriting/test_math.py

@@ -607,7 +607,7 @@ def test_mul_div_cases(self):
            f = function(list(sym_inputs), g, mode=mode)
            out = f(*val_inputs)
            assert out_dtype == out.dtype
-            utt.assert_allclose(out, val_inputs[1])
+            np.allclose(out, val_inputs[1])


These should be np.testing.assert_allclose

ricardoV94 · 2024-10-15T05:36:45Z

tests/tensor/rewriting/test_math.py

@@ -627,7 +627,7 @@ def test_mul_div_cases(self):
        ):
            f = function(list(sym_inputs), g, mode=mode)
            out = f(*val_inputs)
-            utt.assert_allclose(out, (1 / val_inputs[1]))
+            np.allclose(out, (1 / val_inputs[1]))


ricardoV94 · 2024-10-15T05:38:09Z

tests/tensor/test_blockwise.py

-            out, expected_out, atol=1e-6 if config.floatX == "float32" else 0
+            out,
+            expected_out,
+            atol=1e-08 if config.floatX == "float32" else 0,


Why did this change?

ricardoV94 · 2024-10-15T05:45:57Z

tests/tensor/test_nlinalg.py

+        atol_, rtol_ = _get_atol_rtol(np.asarray(t_binv), n_binv)
+        assert np.allclose(t_binv, n_binv, atol=atol_, rtol=rtol_)
+        atol_, rtol_ = _get_atol_rtol(np.asarray(t_binv1), n_binv1)
+        assert np.allclose(t_binv1, n_binv1, atol=atol_, rtol=rtol_)


This is too much code to recreate every time it's used. The question is whether we really need the inner logic. Seems like it has some special way of deciding tolerances for float16/32 complex64. If the tests are covering these types we can hardcode the tolerances, the same way we usually do atol = x if config.floatX else y. But I suspect many of these tests don't even make use of these specific types (I could be wrong).

Otherwise we should keep the original function and not repeat the code so many times in the tests. These functions that are only used in tests should be moved to the tests package, out of the main pytensor package (if we end up keeping it).

Also always use np.testing.assert_allclose and not np.allclose, as it has a nicer output when it fails.

ricardoV94 · 2024-10-15T05:52:07Z

@Dhruvanshu-Joshi This PR is already a pretty good improvement, so I don't want to block it much longer. The main question is what to do with that _allclose that we removed. If we keep using the inner logic everywhere it was used we should keep it. Doesn't make sense to repeat the same code 50 times. Hopefully, however, we don't really need it and can just do np.testing.assert_allclose with hardcoded tolerances based on the types that are actually being tested in each test.

If this is too laborious let's revert and address allclose in another PR

Dhruvanshu-Joshi · 2024-10-30T05:38:12Z

in an

Yep what you mentioned makes complete sense. I try hardcoding the tolerances wherever possible to reduce this unwanted repetitiveness and update whether it is better to go with it or revert this.

Dhruvanshu-Joshi force-pushed the deprecate_testingnp branch 5 times, most recently from 94bdd55 to ba18c65 Compare June 3, 2024 06:48

ricardoV94 reviewed Jun 19, 2024

View reviewed changes

tests/unittest_tools.py Show resolved Hide resolved

ricardoV94 reviewed Jun 19, 2024

View reviewed changes

tests/scan/test_rewriting.py Outdated Show resolved Hide resolved

Dhruvanshu-Joshi force-pushed the deprecate_testingnp branch 3 times, most recently from 135ff93 to 87c6ee6 Compare July 2, 2024 19:08

ricardoV94 reviewed Jul 3, 2024

View reviewed changes

Dhruvanshu-Joshi force-pushed the deprecate_testingnp branch 2 times, most recently from 81a97df to 26782d2 Compare October 2, 2024 17:19

Dhruvanshu-Joshi added 2 commits October 12, 2024 16:30

Deprecate redefinition of np.testing.assert_allclose

d7c7749

Deprecated redefinition of np.allclose in _allclose

035139e

Dhruvanshu-Joshi force-pushed the deprecate_testingnp branch from 26782d2 to 035139e Compare October 12, 2024 11:01

Switch to numpy assert_allclose

6824a20

Dhruvanshu-Joshi force-pushed the deprecate_testingnp branch from abc44ab to 6824a20 Compare October 12, 2024 12:22

ricardoV94 requested changes Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate redefinition of `np.testing.assert_allclose` #784

Deprecate redefinition of `np.testing.assert_allclose` #784

Dhruvanshu-Joshi commented May 27, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading

ricardoV94 left a comment

ricardoV94 Jul 3, 2024

ricardoV94 Jul 3, 2024

Dhruvanshu-Joshi commented Jul 15, 2024

Dhruvanshu-Joshi commented Oct 3, 2024

ricardoV94 Oct 15, 2024

ricardoV94 Oct 15, 2024

ricardoV94 Oct 15, 2024

ricardoV94 Oct 15, 2024

ricardoV94 commented Oct 15, 2024 •

edited

Loading

Dhruvanshu-Joshi commented Oct 30, 2024

Deprecate redefinition of np.testing.assert_allclose #784

Are you sure you want to change the base?

Deprecate redefinition of np.testing.assert_allclose #784

Conversation

Dhruvanshu-Joshi commented May 27, 2024 • edited Loading

Description

Related Issue

Checklist

Type of change

codecov bot commented Jun 3, 2024 • edited Loading

Codecov Report

ricardoV94 left a comment

Choose a reason for hiding this comment

ricardoV94 Jul 3, 2024

Choose a reason for hiding this comment

ricardoV94 Jul 3, 2024

Choose a reason for hiding this comment

Dhruvanshu-Joshi commented Jul 15, 2024

Dhruvanshu-Joshi commented Oct 3, 2024

ricardoV94 Oct 15, 2024

Choose a reason for hiding this comment

ricardoV94 Oct 15, 2024

Choose a reason for hiding this comment

ricardoV94 Oct 15, 2024

Choose a reason for hiding this comment

ricardoV94 Oct 15, 2024

Choose a reason for hiding this comment

ricardoV94 commented Oct 15, 2024 • edited Loading

Dhruvanshu-Joshi commented Oct 30, 2024

Deprecate redefinition of `np.testing.assert_allclose` #784

Deprecate redefinition of `np.testing.assert_allclose` #784

Dhruvanshu-Joshi commented May 27, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading

ricardoV94 commented Oct 15, 2024 •

edited

Loading