Migrate tests/test_aampi.py to npt.assert_allclose - #1187
Merged
Conversation
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1187 |
Contributor
Author
|
@seanlaw next file in the split — same conventions as the previous PRs. Ready for review whenever you have time. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_equalonly checks a fixed absolute tolerance and NumPy's docs recommendassert_allcloseinstead. Renamedcomp_P/comp_I/comp_left_P/comp_left_Itocmp_*per the standing convention, and flipped every call to theactual, desiredorder. The tricky part wasdtype=objectcasting, which only applies to some of the functions here depending on whereref_*comes from:ref_P/ref_I/ref_left_Iby slicing columns out ofnaive.aamp(...)(e.g.ref_mp[:, 0]) — that staysdtype=objecteven after column-slicing (same root cause astest_aamp.py/test_aamped.py), so these needed.astype(np.float64)on both sides.ref_P/ref_I/ref_left_P/ref_left_Ifromnaive.aampi_egress(...).P_/.I_/etc — a dedicated streaming reference implementation that returns plainfloat64/int64directly, no casting needed.test_aampi_profile_index_matchdoesn't use theref_/comp_convention at all — it built localP/left_Parrays and compared them directly againststream.P_/stream.left_P_(already in the correctactual, desiredorder). Renamed those two locals toref_P/ref_left_Pfor consistency with the rest of the file.decimal=config.STUMPY_TEST_PRECISION— kept those asatol=1.5 * 10**-config.STUMPY_TEST_PRECISIONrather than the1.5e-07literal, 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:npt.assert_almost_equalwith its equivalentnpt.assert_allclosertol=0fornpt.assert_allclose, simply omit this parameter and value since this is the defaultnpt.assert_allclose(actual, desired)always has the stumpy computed value asactualand the naive computation asdesiredatol=1.5e-07rather thanatol=1.5*10**-07- applies to the sites using the implicit default; theconfig.STUMPY_TEST_PRECISION-referencing sites keep that reference1.5*10**-config.STUMPY_TEST_PRECISIONwithconfig.STUMPY_TEST_PRECISION = 1.5e-07(inconfig.py) - tracked as a follow-up, not part of this per-file migrationcmpinstead ofcomp- this file usedcomp_P/comp_I/comp_left_P/comp_left_Ithroughout, all renamedref_and stumpy-computed outputs tocmp_(applies tonpt.assert_array_equaltoo) - this file already usedref_/comp_, noassert_array_equalcalls present; also renamed the one function that used bareP/left_PinsteadPull 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 locallyPlease 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!