Fixed #1175 Migrate tests/test_aamp.py to npt.assert_allclose

[viknesh-ai]

Fixed #1175

Per review feedback, split into one PR per test file - this one covers tests/test_aamp.py only. 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. Every comparison in this file is against ref_mp/cmp_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, since the values are always numeric so the cast is exact. All 27 call sites in this file are now on assert_allclose, none left behind. rtol is left at its default rather than pinned to 0.

To Do List

Standing checklist from @seanlaw for this migration, applies to every file in the split - status below is for test_aamp.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 for the computed/performant value, so it's the same length as ref (comp_mp -> cmp_mp)

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!

[gitnotebooks]

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

[seanlaw]

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

[seanlaw]

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

[viknesh-ai]

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

[seanlaw]

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

[viknesh-ai]

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

[seanlaw]

Please ensure that npt.assert_allclose(actual, desired) always has the stumpy computed value as "actual" and the naive computation as "desired".

Currently, in many cases, I am seeing

npt.assert_allclose(
    ref_mp.astype(np.float64),
    cmp_mp.astype(np.float64),
    atol=1.5e-07
)

but it should be flipped to:

npt.assert_allclose(
    cmp_mp.astype(np.float64),
    ref_mp.astype(np.float64),
    atol=1.5e-07
)

Also, please be consistent and use atol=1.5e-07 rather than atol=1.5*10**-07

[seanlaw]

You've removed my edits in your original post so I am repeating it hear for other PRs:

To Do List

• 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 paramter 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)
• Replace comp with cmp

[viknesh-ai]

@seanlaw all comments addressed and replied to — arg order flipped, atol literal style fixed, config.STUMPY_TEST_PRECISION reference restored, and 27/27 tests passing in both normal and CI-equivalent modes. Ready for another look whenever you have time.

[seanlaw]

@viknesh-ai Everything looks good. Is this ready to be merged?

[viknesh-ai]

@viknesh-ai Everything looks good. Is this ready to be merged?

Yes, this one's ready to merge on my end. Once it's merged, can I open the next PR for the next file in the migration?

[seanlaw]

Once it's merged, can I open the next PR for the next file in the migration?

@viknesh-ai Yes, please and thank you.