Skip to content

Fixed #1111 Add branching logic for sdp - #1200

Open
NimaSarajpoor wants to merge 2 commits into
stumpy-dev:mainfrom
NimaSarajpoor:sdp_branching_logic
Open

Fixed #1111 Add branching logic for sdp#1200
NimaSarajpoor wants to merge 2 commits into
stumpy-dev:mainfrom
NimaSarajpoor:sdp_branching_logic

Conversation

@NimaSarajpoor

@NimaSarajpoor NimaSarajpoor commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

See #1111

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
    • less than 25% of the code in this pull request
    • 25-50% of the code in this pull request
    • more than 50% 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!


This is to address PR 4 as described in #1118 (comment). I've copied the corresponding notes below:

Notice that we haven't utilized ANY of the _X_sliding_dot_product anywhere in the code except for _njit_sliding_dot_product but that hasn't altered any of the code in a meaningful way as it was purely a refactor. Only at the very end in this PR do we start working on allowing the other _X_sliding_dot_product to be used. This is where we focus on the branching logic!

Note that instead of core.sliding_dot_product = sdp._sliding_dot_product, we can redefine core.sliding_dot_product so that it performs checks (e.g., check whether pyfftw is installed BEFORE callings sdp._sliding_dot_product). This way, sdp._sliding_dot_product can make assumptions that the user knows what they're doing while core.sliding_dot_product is slower but is super safe to use (hence it is public)!

@NimaSarajpoor
NimaSarajpoor requested a review from seanlaw as a code owner August 25, 2026 05:19
@gitnotebooks

gitnotebooks Bot commented Aug 25, 2026

Copy link
Copy Markdown

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

@NimaSarajpoor NimaSarajpoor changed the title Add branching logic for sdp Fixed #1111 Add branching logic for sdp Aug 25, 2026
@NimaSarajpoor

Copy link
Copy Markdown
Collaborator Author

As discussed previously in this comment, I am planning to replace core.sliding_dot_product with something like this:

def sliding_dot_product(Q, T, boundaries):      
      m = len(Q)
      n = len(T)
      sdp = None
      for boundary in boundaries:
            (LB_m, UB_m), (LB_n, UB_n), sdp_func = boundary
             if LB_m <= m < UB_m and LB_n <= n < UB_n:
                  return sdp_func(Q, T)
           
      return sdp_func_default(Q, T)

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.

1 participant