Refactor/sdp structure cleanup by NimaSarajpoor · Pull Request #1126 · stumpy-dev/stumpy

NimaSarajpoor · 2026-02-07T01:48:21Z

This is to address PR 1 described in this comment. Have copied the corresponding notes below:

Create a new sdp.py file

Move _sliding_dot_product to sdp.py and call it _njit_sliding_dot_product

Removing the core._sliding_dot_product altogether (currently, you are keeping both and calling one from the other. This is a bad idea)

Replace all (three) references to core._sliding_dot_product with sdp._njit_sliding_dot_product

Update move tests from tests/test_core.py to tests/test_sdp.py

Add an sdp._sliding_dot_product that simply calls _njit_sliding_dot_product (complexity gets added in future PRs)

Make core.sliding_dot_product = sdp._sliding_dot_product

gitnotebooks · 2026-02-07T01:48:24Z

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

NimaSarajpoor · 2026-02-07T02:10:47Z

stumpy/core.py

-    QT = convolve(Qr, T)
-
-    return QT.real[m - 1 : n]
+    return sdp._sliding_dot_product(Q, T)


@seanlaw
Regarding core.sliding_dot_prodiuct: Is this what you meant when you said in this comment (see notes on PR 1):

Make core.sliding_dot_product = sdp._sliding_dot_product

?

I think we should handle sdp._convolve_sliding_dot_product (see PR 2 in this comment) in this PR. The function core.sliding_dot_product in the branch main can be copied to the sdp module, and renamed to _convolve_sliding_dot_product. sdp._sliding_dot_product should point to sdp._convolve_sliding_dot_product. This is not a serious issue if the plan is to handle PR 2 quickly right after PR 1.

Yes, that makes perfect sense. It's not actually new code or new SDP methods. It's simply a refactoring of existing code

sdp._sliding_dot_product should point to sdp._convolve_sliding_dot_product

And then, in subsequent PRs, it should point to multiple functions using some branching logic, right?

Correct 👍

NimaSarajpoor · 2026-02-07T02:13:23Z

tests/test_sdp.py

+from stumpy import sdp
+
+
+def naive_rolling_window_dot_product(Q, T):


We have a similar function in tests/test_core.py. Refactor and put this function in tests/naive.py.

The only reason why I didn't want to put it in naive.py was because it wasn't actually being used in any other file except for test_core.py. The only time I would move a function to naive.py is when it was used across multiple test files.

The only time I would move a function to naive.py is when it was used across multiple test files.

Noted. Since I am now using it in tests/test_sdp.py too, it makes sense to just move it to naive.py. Is that correct?

NimaSarajpoor · 2026-02-07T02:34:42Z

@seanlaw
We also need to modify Tutorial_DiscordMERLIN.ipynb as it uses core._sliding_dot_product, and need to run the whole notebook again. However, the problem is that the notebook loads data from directories that do not exist. Also, the data provided by MERLIN webpage is not accessible anymore.

seanlaw

@NimaSarajpoor Looking good! This is much easier to track

seanlaw · 2026-02-07T03:14:57Z

stumpy/core.py

-    QT = convolve(Qr, T)
-
-    return QT.real[m - 1 : n]
+    return sdp._sliding_dot_product(Q, T)


Yes, that makes perfect sense. It's not actually new code or new SDP methods. It's simply a refactoring of existing code

seanlaw · 2026-02-07T03:17:59Z

stumpy/core.py

                T_squared[i - 1] - T[i - 1] * T[i - 1] + T[i + m - 1] * T[i + m - 1]
            )
-        QT = _sliding_dot_product(Q, T)
+        QT = sdp._njit_sliding_dot_product(Q, T)


With the name being sdp._njit_sliding_dot_product, it actually makes logical sense. This _p_norm_distance_profile is an NJIT decorated function and, therefore, it MUST use an NJIT capable SDP function! I like that this distinction is clear and obvious

While core.sliding_dot_product is NOT necessarily an NJIT function (it could be but doesn't have to be).

I like that this distinction is clear and obvious

While core.sliding_dot_product is NOT necessarily an NJIT function (it could be but doesn't have to be).

YES, and YES!!!

seanlaw · 2026-02-07T03:21:17Z

stumpy/core.py

-    QT = convolve(Qr, T)
-
-    return QT.real[m - 1 : n]
+    return sdp._sliding_dot_product(Q, T)


sdp._sliding_dot_product should point to sdp._convolve_sliding_dot_product

And then, in subsequent PRs, it should point to multiple functions using some branching logic, right?

seanlaw · 2026-02-07T03:23:19Z

tests/test_sdp.py

+from stumpy import sdp
+
+
+def naive_rolling_window_dot_product(Q, T):


The only reason why I didn't want to put it in naive.py was because it wasn't actually being used in any other file except for test_core.py. The only time I would move a function to naive.py is when it was used across multiple test files.

NimaSarajpoor added 2 commits February 6, 2026 00:31

added new module sdp

29ea35d

Move sliding-dot-product from core to new module sdp

d0b2375

NimaSarajpoor requested a review from seanlaw as a code owner February 7, 2026 01:48

NimaSarajpoor commented Feb 7, 2026

View reviewed changes

seanlaw requested changes Feb 7, 2026

View reviewed changes

		from stumpy import sdp


		def naive_rolling_window_dot_product(Q, T):

Conversation

NimaSarajpoor commented Feb 7, 2026

Uh oh!

gitnotebooks bot commented Feb 7, 2026

Uh oh!

NimaSarajpoor Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NimaSarajpoor commented Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanlaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NimaSarajpoor Feb 7, 2026 •

edited

Loading

NimaSarajpoor commented Feb 7, 2026 •

edited

Loading