Migrate tests from npt.assert_almost_equal to npt.assert_allclose - #1178
Migrate tests from npt.assert_almost_equal to npt.assert_allclose#1178viknesh-ai wants to merge 1 commit into
Conversation
npt.assert_almost_equal only checks a fixed absolute tolerance (abs(actual-desired) < 1.5 * 10**-decimal), which NumPy's own docs recommend against in favor of assert_allclose. Swap every call site to assert_allclose(a, b, atol=<same threshold>, rtol=0), which preserves today's exact pass/fail behavior with zero risk. A few hundred call sites compare arrays that combine a float distance column with an int index column (matrix profile output, match/motifs results, stumpi KNN state). Those end up dtype=object, and np.isclose can't handle object dtype the way assert_almost_equal can, so those specific comparisons are left on assert_almost_equal. Verified this is genuinely data-dependent (not just "avoid P_/I_ everywhere") by running the full suite the same way test.sh coverage does - one pytest process per file, JIT disabled, CUDA simulated - and reverting only the sites that actually broke. Fixes stumpy-dev#1175
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1178 |
seanlaw
left a comment
seanlaw
left a comment
There was a problem hiding this comment.
Instead of submitting a single PR that touches 38 files at once, let's split this into one PR per test file. Only submit a new PR after the last PR gets merged (i.e., do NOT submit 38 PRs all at the same time).
Also, please ensure that:
- Each test file correctly replaces all uses of
npt.assert_almost_equalwith its equivalentnpt.assert_allclose - When
rtol=0fornpt.assert_allclose, simply omit this paramter and value since this is the default
| @@ -132,13 +132,17 @@ def test_aamp_identical_subsequence_self_join(): | |||
| naive.replace_inf(ref_mp) | |||
| naive.replace_inf(comp_mp) | |||
| npt.assert_almost_equal( | |||
There was a problem hiding this comment.
Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?
|
|
||
| comp_mp = aamp(pd.Series(T_A), m, ignore_trivial=True) | ||
| naive.replace_inf(comp_mp) | ||
| npt.assert_almost_equal( |
There was a problem hiding this comment.
Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?
| @@ -154,13 +158,17 @@ def test_aamp_identical_subsequence_A_B_join(): | |||
| naive.replace_inf(ref_mp) | |||
| naive.replace_inf(comp_mp) | |||
| npt.assert_almost_equal( | |||
There was a problem hiding this comment.
Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?
|
|
||
| comp_mp = aamp(pd.Series(T_A), m, pd.Series(T_B), ignore_trivial=False) | ||
| naive.replace_inf(comp_mp) | ||
| npt.assert_almost_equal( |
There was a problem hiding this comment.
Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?
| @@ -169,7 +177,9 @@ def test_aamp_identical_subsequence_A_B_join(): | |||
| naive.replace_inf(ref_mp) | |||
| naive.replace_inf(comp_mp) | |||
| npt.assert_almost_equal( | |||
There was a problem hiding this comment.
Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?
| @@ -285,7 +285,9 @@ def test_aamped_identical_subsequence_self_join(dask_cluster): | |||
| naive.replace_inf(ref_mp) | |||
| naive.replace_inf(comp_mp) | |||
| npt.assert_almost_equal( | |||
There was a problem hiding this comment.
Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?
| @@ -308,7 +310,9 @@ def test_aamped_identical_subsequence_A_B_join(dask_cluster): | |||
| naive.replace_inf(ref_mp) | |||
| naive.replace_inf(comp_mp) | |||
| npt.assert_almost_equal( | |||
There was a problem hiding this comment.
Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?
2 participants
Fixes #1175
npt.assert_almost_equal only checks a fixed absolute tolerance (abs(actual-desired) < 1.5 * 10**-decimal), and NumPy's own docs recommend assert_allclose instead for more consistent floating-point comparisons.
Migrated every call site to assert_allclose(a, b, atol=, rtol=0) — rtol=0 keeps it a pure absolute-tolerance check, so today's exact pass/fail behavior is preserved, just via the non-deprecated API.
A subset of comparisons (matrix profile output, match/motifs results, stumpi KNN state) combine a float distance column with an int index column, so they end up dtype=object. np.isclose can't handle object dtype the way assert_almost_equal can, so those specific sites were left on assert_almost_equal rather than force the migration. Confirmed this is genuinely data-dependent (not just "avoid P_/I_ everywhere") by running the suite the same way test.sh coverage does — one pytest process per file, JIT disabled, CUDA simulated — and only reverting the sites that actually broke.
Pull Request Checklist
Below is a simple checklist but please do not hesitate to ask for assistance!
black(i.e.,python -m pip install blackorconda install -c conda-forge black)flake8(i.e.,python -m pip install flake8orconda install -c conda-forge flake8)pytest-cov(i.e.,python -m pip install pytest-covorconda install -c conda-forge pytest-cov)black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./in the root stumpy directoryflake8 --extend-exclude=.venv ./in the root stumpy directory./setup.sh dev && ./test.shin the root stumpy directory and ensured that all tests are passing locally