Skip to content

Drop redundant k clamp in _mpdist_vect and _aampdist_vect - #1158

Closed
DMZ22 wants to merge 1 commit into
stumpy-dev:mainfrom
DMZ22:simplify-mpdist-k-clamp
Closed

Drop redundant k clamp in _mpdist_vect and _aampdist_vect#1158
DMZ22 wants to merge 1 commit into
stumpy-dev:mainfrom
DMZ22:simplify-mpdist-k-clamp

Conversation

@DMZ22

@DMZ22 DMZ22 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

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)
  • Left a meaningful comment on the original Github issue to discuss the detailed approach for your contribution
  • Forked, cloned, and checkedout 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

On the last item: I'm on Windows, so instead of ./test.sh I ran the two affected suites directly — pytest tests/test_mpdist.py tests/test_aampdist.py82 passed. Happy to run anything else you'd like. black reported 99 files unchanged and flake8 was clean.


Closes #1082.

P_ABBA is allocated as np.empty(2 * j, ...), so P_ABBA.shape[0] - 1 is 2 * j - 1. That means the k = min(int(k), P_ABBA.shape[0] - 1) on the following line already applies exactly the same upper bound as the min(..., 2 * j - 1) inside the if k is None branch, so the inner clamp is redundant:

P_ABBA = np.empty(2 * j, dtype=np.float64)

if k is None:
    percentage = np.clip(percentage, 0.0, 1.0)
    k = math.ceil(percentage * (2 * Q.shape[0]))   # inner min(..., 2*j-1) removed

k = min(int(k), P_ABBA.shape[0] - 1)               # already bounds k by 2*j-1

Applied to aampdist as well, as you noted on the issue.

Why it's behaviour-preserving

min(min(a, b), b) == min(a, b), and the int() is a no-op because math.ceil already returns an int. I also checked it empirically over 20,000 randomised (n, m, percentage) combinations — including percentage values outside [0, 1] so the clipping path is exercised — and the resulting k was identical in every case.

P_ABBA is allocated as np.empty(2 * j), so P_ABBA.shape[0] - 1 is 2 * j - 1.
The k = min(int(k), P_ABBA.shape[0] - 1) that follows therefore already
applies the same upper bound that the min(..., 2 * j - 1) inside the
k is None branch does, making the inner clamp redundant.

Remove it in both mpdist and aampdist, as suggested in the issue.

Closes stumpy-dev#1082.
@DMZ22
DMZ22 requested a review from seanlaw as a code owner July 24, 2026 07:22
@gitnotebooks

gitnotebooks Bot commented Jul 24, 2026

Copy link
Copy Markdown

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

@github-actions

Copy link
Copy Markdown

❌ This pull request has been automatically closed because the mandatory PR template was removed or not filled out. Please reopen or recreate this PR with the required information.

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.

Suggesting a minor enhancement in mpdist.py:: _mpdist_vect

1 participant