Skip to content

Migrate tests/test_aampi.py to npt.assert_allclose - #1187

Merged
seanlaw merged 1 commit into
stumpy-dev:mainfrom
viknesh-ai:fix/1175-migrate-test-aampi
Aug 16, 2026
Merged

Migrate tests/test_aampi.py to npt.assert_allclose#1187
seanlaw merged 1 commit into
stumpy-dev:mainfrom
viknesh-ai:fix/1175-migrate-test-aampi

Conversation

@viknesh-ai

Copy link
Copy Markdown
Contributor

Related to #1175

Continuing the file-by-file split — this one covers tests/test_aampi.py. Largest file in the split so far (138 call sites across 17 test functions), so it's worth explaining the approach.

assert_almost_equal only checks a fixed absolute tolerance and NumPy's docs recommend assert_allclose instead. Renamed comp_P/comp_I/comp_left_P/comp_left_I to cmp_* per the standing convention, and flipped every call to the actual, desired order. The tricky part was dtype=object casting, which only applies to some of the functions here depending on where ref_* comes from:

  • 7 functions build ref_P/ref_I/ref_left_I by slicing columns out of naive.aamp(...) (e.g. ref_mp[:, 0]) — that stays dtype=object even after column-slicing (same root cause as test_aamp.py/test_aamped.py), so these needed .astype(np.float64) on both sides.
  • 8 functions instead build ref_P/ref_I/ref_left_P/ref_left_I from naive.aampi_egress(...).P_/.I_/etc — a dedicated streaming reference implementation that returns plain float64/int64 directly, no casting needed.
  • I checked the actual runtime dtype of every distinct source in this file before writing any of the migration, rather than assuming based on the earlier files, since guessing here would've been easy to get wrong in either direction.
  • test_aampi_profile_index_match doesn't use the ref_/comp_ convention at all — it built local P/left_P arrays and compared them directly against stream.P_/stream.left_P_ (already in the correct actual, desired order). Renamed those two locals to ref_P/ref_left_P for consistency with the rest of the file.
  • 2 functions explicitly pass decimal=config.STUMPY_TEST_PRECISION — kept those as atol=1.5 * 10**-config.STUMPY_TEST_PRECISION rather than the 1.5e-07 literal, same as the earlier files.

Ran the full file twice locally (normal mode and JIT-disabled/CUDA-simulated, matching test.sh coverage) — 21/21 passing both times.

To Do List

Standing checklist from @seanlaw for this migration, applies to every file in the split - status below is for test_aampi.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 - applies to the sites using the implicit default; the config.STUMPY_TEST_PRECISION-referencing sites keep that reference
  • 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 - this file used comp_P/comp_I/comp_left_P/comp_left_I throughout, all renamed
  • Rename naive outputs to ref_ and stumpy-computed outputs to cmp_ (applies to npt.assert_array_equal too) - this file already used ref_/comp_, no assert_array_equal calls present; also renamed the one function that used bare P/left_P instead

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 16, 2026 15:21
@gitnotebooks

gitnotebooks Bot commented Aug 16, 2026

Copy link
Copy Markdown

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

@viknesh-ai

Copy link
Copy Markdown
Contributor Author

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

@seanlaw
seanlaw merged commit 077623b into stumpy-dev:main Aug 16, 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.

2 participants