Skip to content

Fixed #1175 Migrate tests/test_aamp_motifs.py to npt.assert_allclose - #1181

Merged
seanlaw merged 2 commits into
stumpy-dev:mainfrom
viknesh-ai:fix/1175-migrate-test-aamp-motifs
Aug 14, 2026
Merged

Fixed #1175 Migrate tests/test_aamp_motifs.py to npt.assert_allclose#1181
seanlaw merged 2 commits into
stumpy-dev:mainfrom
viknesh-ai:fix/1175-migrate-test-aamp-motifs

Conversation

@viknesh-ai

@viknesh-ai viknesh-ai commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixed #1175

Continuing the file-by-file split from #1178 — this one covers tests/test_aamp_motifs.py. Will open the next file's PR once this one merges.

assert_almost_equal only checks a fixed absolute tolerance and NumPy's docs recommend assert_allclose instead. Eight call sites in this file, three different wrinkles:

  • Two calls compare the left/right argument order backwards relative to the actual, desired convention (right/out is always the stumpy-computed value in this file, left is either a hand-derived expected value or the naive helper's output) — flipped both.
  • Four calls compare against aamp_match/naive_aamp_match output, which mixes a float distance with an int index and so comes back dtype=object — same issue as test_aamp.py, same fix: cast both sides to float64 before comparing, since the values are always numeric.
  • The original decimal= values weren't all the same (4, 6, and the implicit default of 7), so I kept each site's original tolerance rather than flattening everything to 1.5e-07 — translated decimal=4/decimal=6 to atol=1.5e-04/atol=1.5e-06 respectively, and the two unlabeled ones to atol=1.5e-07 per the usual convention.

Also ran the file across five random seeds (JIT disabled, CUDA simulated, matching test.sh coverage) since test_data in this file draws from unseeded rng.RNG — wanted to make sure the tolerances hold up beyond just the one seed I happened to run with.

Update: per review, also renamed every left/right variable to ref_/cmp_ (naive output = ref, stumpy-computed output = cmp), and flipped the npt.assert_array_equal argument order to match the same actual, desired convention as assert_allclose.

To Do List

Standing checklist from @seanlaw for this migration, applies to every file in the split - status below is for test_aamp_motifs.py:

  • Each test file correctly replaces all uses of npt.assert_almost_equal with its equivalent npt.assert_allclose
  • When rtol=0 for npt.assert_allclose, simply omit this parameter and value since this is the default
  • Ensure that npt.assert_allclose(actual, desired) always has the stumpy computed value as actual and the naive computation as desired
  • Use atol=1.5e-07 rather than atol=1.5*10**-07
  • Eventually, replace the ugly 1.5*10**-config.STUMPY_TEST_PRECISION with config.STUMPY_TEST_PRECISION = 1.5e-07 (in config.py) - tracked as a follow-up, not part of this per-file migration
  • Use cmp instead of comp - n/a for this file, it never used comp naming to begin with
  • Rename naive outputs to ref_ and stumpy-computed outputs to cmp_ (applies to npt.assert_array_equal too, not just npt.assert_allclose) - this file originally used left/right, all renamed

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

Please do not commit any code to avoid/circumvent a failing test and, instead, engage in a discussion (below) to determine the best course of action.

Only request a review after the checklist above is fully completed!

@viknesh-ai
viknesh-ai requested a review from seanlaw as a code owner August 13, 2026 13:11
@gitnotebooks

gitnotebooks Bot commented Aug 13, 2026

Copy link
Copy Markdown

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

@viknesh-ai

Copy link
Copy Markdown
Contributor Author

@seanlaw next file in the split — same conventions as #1178. Ready for review whenever you have time.

Comment thread tests/test_aamp_motifs.py Outdated
Rename left/right to ref/cmp for consistency, and flip assert_array_equal
argument order to match the actual, desired convention.
@viknesh-ai
viknesh-ai requested a review from seanlaw August 13, 2026 18:22

@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.

@viknesh-ai Looks good to me. Ready to merge?

@viknesh-ai

Copy link
Copy Markdown
Contributor Author

@viknesh-ai Looks good to me. Ready to merge?

Yeah It’s Ready to merge!

@seanlaw
seanlaw merged commit 699bd45 into stumpy-dev:main Aug 14, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate to npt.assert_allclose

2 participants