Skip to content

Migrate tests from npt.assert_almost_equal to npt.assert_allclose - #1178

Open
viknesh-ai wants to merge 1 commit into
stumpy-dev:mainfrom
viknesh-ai:fix/1175-migrate-to-assert-allclose
Open

Migrate tests from npt.assert_almost_equal to npt.assert_allclose#1178
viknesh-ai wants to merge 1 commit into
stumpy-dev:mainfrom
viknesh-ai:fix/1175-migrate-to-assert-allclose

Conversation

@viknesh-ai

@viknesh-ai viknesh-ai commented Aug 9, 2026 •
edited
Loading

Copy link
Copy Markdown

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!

  • Read our Contributing Guide
  • Referenced a Github issue (or create one if one doesn't already exist)
  • Read and reviewed all of the comments in the Github issue that you've referenced (along with cross-referenced issues/pull requests) to ensure that the issue still requires a pull request
  • Checked that the issue has not already been assigned to anybody else or is already being addressed in another pull request
  • Left a meaningful comment on the original Github issue to discuss the detailed approach for your contribution and received confirmation from the maintainers before proceeding with this pull request
  • Forked, cloned, and checked out the newest version of the code
  • Created a new branch
  • Made necessary code changes
  • Installed black (i.e., python -m pip install black or conda install -c conda-forge black)
  • Installed flake8 (i.e., python -m pip install flake8 or conda install -c conda-forge flake8)
  • Installed pytest-cov (i.e., python -m pip install pytest-cov or conda install -c conda-forge pytest-cov)
  • Ran black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./ in the root stumpy directory
  • Ran flake8 --extend-exclude=.venv ./ in the root stumpy directory
  • Ran ./setup.sh dev && ./test.sh in the root stumpy directory and ensured that all tests are passing locally
  • Check this box if AI code assistance was used to generate 15%+ of the code in this pull request

@viknesh-ai
viknesh-ai requested a review from seanlaw as a code owner August 9, 2026 17:41
@gitnotebooks

gitnotebooks Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1178

seanlaw
seanlaw requested changes Aug 9, 2026
View reviewed changes

@seanlaw seanlaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Each test file correctly replaces all uses of npt.assert_almost_equal with its equivalent npt.assert_allclose
  2. When rtol=0 for npt.assert_allclose, simply omit this paramter and value since this is the default

Comment thread tests/test_aamp.py Outdated
@@ -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(

@seanlaw seanlaw Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?

@viknesh-ai viknesh-ai Aug 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that was a workaround, not a real fix. The reason it was still assert_almost_equal is that ref_mp/comp_mp here mix a float distance column with int index columns, so the array comes back dtype=object, and np.isclose can't handle object dtype the way assert_almost_equal can. Instead of leaving it on the deprecated API, I pushed a version that casts both sides to float64 before comparing (values are always numeric, so the cast is exact) — everything in this file is on assert_allclose now

Comment thread tests/test_aamp.py Outdated

comp_mp = aamp(pd.Series(T_A), m, ignore_trivial=True)
naive.replace_inf(comp_mp)
npt.assert_almost_equal(

@seanlaw seanlaw Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?

Comment thread tests/test_aamp.py Outdated
@@ -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(

@seanlaw seanlaw Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?

Comment thread tests/test_aamp.py Outdated

comp_mp = aamp(pd.Series(T_A), m, pd.Series(T_B), ignore_trivial=False)
naive.replace_inf(comp_mp)
npt.assert_almost_equal(

@seanlaw seanlaw Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?

Comment thread tests/test_aamp.py Outdated
@@ -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(

@seanlaw seanlaw Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?

Comment thread tests/test_aamped.py
comp_mp = aamped(dask_client, T_A, m, ignore_trivial=True)
naive.replace_inf(ref_mp)
naive.replace_inf(comp_mp)
npt.assert_almost_equal(

@seanlaw seanlaw Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?

@viknesh-ai viknesh-ai Aug 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same root cause as the test_aamp.py threads — will fix it the same way (cast to float64 before comparing). Per your note about splitting this into one PR per file, I've scoped this PR down to just test_aamp.py for now; test_aamped.py will get its own PR once this one's merged

Comment thread tests/test_aamped.py
comp_mp = aamped(dask_client, T_A, m, T_B, ignore_trivial=False)
naive.replace_inf(ref_mp)
naive.replace_inf(comp_mp)
npt.assert_almost_equal(

@seanlaw seanlaw Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is npt.assert_almost_equal still being used here instead of npt.assert_allclose?

@viknesh-ai
Migrate tests/test_aamp.py from npt.assert_almost_equal to npt.assert…
eafcae4 
…_allclose

npt.assert_almost_equal only checks a fixed absolute tolerance, and
NumPy's docs recommend assert_allclose instead. Every comparison in
this file is against `ref_mp`/`comp_mp` (or a column slice of it),
which combine a float distance column with int index columns and so
come back dtype=object - np.isclose can't handle that directly. Cast
both sides to float64 before comparing instead of leaving these on
the deprecated API; the values are always numeric so the cast is
exact. rtol is left at its default rather than pinned to 0.

First of a per-file split of stumpy-dev#1175, per review feedback.
@viknesh-ai
viknesh-ai force-pushed the fix/1175-migrate-to-assert-allclose branch from d33405c to eafcae4 Compare August 10, 2026 12:56
@viknesh-ai
viknesh-ai requested a review from seanlaw August 10, 2026 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanlaw seanlaw Awaiting requested review from seanlaw seanlaw is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate to npt.assert_allclose

2 participants

@viknesh-ai @seanlaw