Fixed #1111 Add module for sliding dot product; Include pyfftw as (soft) dependency

[NimaSarajpoor]

See #1111

Pull Request Checklist

Below is a simple checklist but please do not hesitate to ask for assistance!

• Fork, clone, and checkout the newest version of the code
• Create a new branch
• Make necessary code changes
• Install black (i.e., python -m pip install black or conda install -c conda-forge black)
• Install flake8 (i.e., python -m pip install flake8 or conda install -c conda-forge flake8)
• Install pytest-cov (i.e., python -m pip install pytest-cov or conda install -c conda-forge pytest-cov)
• Run black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./ in the root stumpy directory
• Run flake8 --extend-exclude=.venv ./ in the root stumpy directory
• Run ./setup.sh dev && ./test.sh in the root stumpy directory
• Reference a Github issue (and create one if one doesn't already exist)

[gitnotebooks]

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

[NimaSarajpoor]

@seanlaw
As suggested in this comment, I will create a module for sliding dot product in this PR, and put the functions/classes there. I've started with changing the current "sliding dot product" functions in core.py to help us follow the changes more easily.

[seanlaw]

Your changes look good to me

[NimaSarajpoor]

I will create a module for sliding dot product in this PR,

seanlaw approved these changes

To make sure we are on the same page, I assume it is still okay to push changes here in this PR.

[seanlaw]

To make sure we are on the same page, I assume it is still okay to push changes here in this PR.

Yes, should be fine. If you feel like there are multiple distinct pieces or logical checkpoints then you may consider splitting it into multiple PRs. But if you are able to keep it simple, then we can do it here

[NimaSarajpoor]

Add fftw and pyfftw to pyproject.toml as well (and Github Actions need to install the dependencies via pixi/conda ??)

[seanlaw]

Note that fftw and pyfftw already exists in pyproject.toml:

stumpy/pyproject.toml

Lines 89 to 90 in ce05903

	fftw = "*"
	pyfftw = "*"

I only added it to the pixi section because it can be easily installed via conda. I don't think the same is true for pip

[NimaSarajpoor]

I remember our previous conversation regarding FFTW and the fact that it cannot be installed via pip since it is not a Python library. My intention was to just install pyFFTW and see if we can test it in one or two platforms if they already have FFTW. I think that is pointless! It's time to revise GitHub Actions (to make sure all dependencies are installed properly and tests can pass in all 3 platforms with full coverage)

[seanlaw]

@NimaSarajpoor It's not clear why we need to change docstring.py and how it relates to sdp. Perhaps we can handle anything that is wrong with docstring.py as a separate issue along with a unit test or clear example where it is currently failing?

The if-condition was changed from class_name is None to len(re.findall(r"Returns", docstring)) > 0. Because, with old condition, this line resulted in wrong outcome when it tries to capture param_section from a docstring that has an at least one output in its "Returns" section.

Frankly, I'm not following this comment and why it needs to be changed.

[NimaSarajpoor]

The if-condition was changed from class_name is None to len(re.findall(r"Returns", docstring)) > 0. Because, with old condition, this line resulted in wrong outcome when it tries to capture param_section from a docstring that has an at least one output in its "Returns" section.

Frankly, I'm not following this comment and why it needs to be changed

and how it relates to sdp

The issue was exposed when I added pyfftw-based sdp, which is implemented using class. The class has the method __call__, which returns a value. Note that NO method in OTHER (EXISTING) classes in STUMPY returns a value, and that's why that issue was not exposed before. When method returns a value, its corresponding variable name (mentioned in the "Returns" section) was MISTAKENLY treated as "parameters" by docstring.py. See the else part in the following if-else block:

stumpy/docstring.py

Lines 26 to 33 in ce05903

	if class_name is None:
	params_section = re.findall(
	r"(?<=Parameters)(.*)(?=Returns)", docstring, re.DOTALL
	)[0]
	else:
	params_section = re.findall(r"(?<=Parameters)(.*)", docstring, re.DOTALL)[0]
	args = re.findall(r"(\w+)\s+\:", params_section)

Perhaps we can handle anything that is wrong with docstring.py as a separate issue along with a unit test or clear example where it is currently failing?

Sounds good. I will create an issue for doscstirng.py

[seanlaw]

The issue was exposed when I added pyfftw-based sdp, which is implemented using class. The class has the method call, which returns a value. Note that NO method in OTHER (EXISTING) classes in STUMPY returns a value, and that's why that issue was not exposed before. When method returns a value, its corresponding variable name (mentioned in the "Returns" section) was MISTAKENLY treated as "parameters" by docstring.py.

Given that this is an extreme case, it almost feels like it merits its on elif condition that specifically handles the specific __call__ case? Then, if the code looks so similar to the existing example handling class then we can combine it thereafter. But you should demonstrate the similarity first rather than getting too cute and obfuscating the logic. Considering how infrequent this function gets used and it is only a helper script, speed is NOT the goal here. Clarity and ease of making modifications is more important!

P.S. I can also accept the criticism (for myself) if this file was written in a way that lacked clarity and if it felt hard to modify 😅

[NimaSarajpoor]

The issue was exposed when I added pyfftw-based sdp, which is implemented using class. The class has the method call, which returns a value. Note that NO method in OTHER (EXISTING) classes in STUMPY returns a value, and that's why that issue was not exposed before. When method returns a value, its corresponding variable name (mentioned in the "Returns" section) was MISTAKENLY treated as "parameters" by docstring.py.

Given that this is an extreme case, it almost feels like it merits its on elif condition that specifically handles the specific __call__ case? Then, if the code looks so similar to the existing example handling class then we can combine it thereafter. But you should demonstrate the similarity first rather than getting too cute and obfuscating the logic. Considering how infrequent this function gets used and it is only a helper script, speed is NOT the goal here. Clarity and ease of making modifications is more important!

P.S. I can also accept the criticism (for myself) if this file was written in a way that lacked clarity and if it felt hard to modify 😅

Let's continue this conversation in the new issue #1123.