Fixed #1111 Add module for sliding dot product; Include pyfftw as (soft) dependency#1118
Fixed #1111 Add module for sliding dot product; Include pyfftw as (soft) dependency#1118NimaSarajpoor wants to merge 27 commits intostumpy-dev:mainfrom
Conversation
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1118 |
|
@seanlaw |
seanlaw
left a comment
There was a problem hiding this comment.
Your changes look good to me
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 It's not clear why we need to change
Frankly, I'm not following this comment and why it needs to be changed. |
The issue was exposed when I added pyfftw-based sdp, which is implemented using class. The class has the method Lines 26 to 33 in ce05903
Sounds good. I will create an issue for |
Given that this is an extreme case, it almost feels like it merits its on 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. |
|
The following error was raised in |
|
This test failed and the error is provided below. It shows |
@NimaSarajpoor I am seeing this error more frequently. Just re-run the test and ignore. Somehow, it is timing out when it is trying to close the cluster. I will look into it but it is very hard to reproduce because this seems to be related to the unpredictable environment in Github Actions. |
NimaSarajpoor
left a comment
There was a problem hiding this comment.
@seanlaw
I think there is nothing more to add to this PR. I left some comments to bring your attention to a few things. Can you please take a look at your convenience?
| _pyfftw_sliding_dot_product = _PYFFTW_SLIDING_DOT_PRODUCT(max_n=2**20) | ||
|
|
||
|
|
||
| def _sliding_dot_product( |
There was a problem hiding this comment.
A couple of notes for this function:
(1) I do not check if FFTW_IS_AVAILABLE in this function. Note that if FFTW_IS_AVAILABLE==False, there will be no function _pyfftw_sliding_dot_product in this module. Whatever functions a user provides in boundaries, the assumption is that it works and it returns the correct output.
(2) The default_sdp is set to the function _convolve_sliding_dot_product, which is core.sliding_dot_product in main branch.
| Q, | ||
| T, | ||
| boundaries=[ | ||
| [(-np.inf, 2**7 + 1), (-np.inf, np.inf), _njit_sliding_dot_product], |
There was a problem hiding this comment.
| [(-np.inf, 2**7 + 1), (-np.inf, np.inf), _njit_sliding_dot_product], | |
| [(2, 2**7 + 1), (2, np.inf), _njit_sliding_dot_product], |
| Q_boundaries[0] <= m < Q_boundaries[1] | ||
| and T_boundaries[0] <= n < T_boundaries[1] | ||
| ): | ||
| return sdp_func(Q, T) |
There was a problem hiding this comment.
what if a user wants to use the function sdp._pocketfft_sliding_dot_product which uses some private functions of scipy? and that might break the function... Should we care if PyPI Wheel is checked on daily basis ?
| QT = convolve(Qr, T) | ||
|
|
||
| return QT.real[m - 1 : n] | ||
| return sdp._sliding_dot_product(Q, T) |
There was a problem hiding this comment.
FYI: If we pass boundaries=[], then this will be the same as the function in branch main. However, I think it is safe to use the default value, which is:
boundaries=[
[(-np.inf, 2**7 + 1), (-np.inf, np.inf), _njit_sliding_dot_product],
]
Alright, as I am going through it, there are too many things happening at the same time and I'm having trouble keeping it all in my head. Sorry for that! @NimaSarajpoor May I ask that you break this up into separate PRs? Once one PR is complete, we can sequentially move on to the next one: PR 1
PR 2
PR 3
PR 4Notice that we haven't utilized ANY of the Note that instead of Does that make sense? |
Sure... Thanks for putting time into breaking that down. That was a great lesson for me!!!
I think I am good for the most part. I am going to close this PR, and then start with handling those PRs one by one. |
See #1111
Pull Request Checklist
Below is a simple checklist but please do not hesitate to ask for assistance!
black(i.e.,python -m pip install blackorconda install -c conda-forge black)flake8(i.e.,python -m pip install flake8orconda install -c conda-forge flake8)pytest-cov(i.e.,python -m pip install pytest-covorconda install -c conda-forge pytest-cov)black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./in the root stumpy directoryflake8 --extend-exclude=.venv ./in the root stumpy directory./setup.sh dev && ./test.shin the root stumpy directory