-
Notifications
You must be signed in to change notification settings - Fork 341
Fixed #1111 Add module for sliding dot product; Include pyfftw as (soft) dependency #1118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fixed #1111 Add module for sliding dot product; Include pyfftw as (soft) dependency #1118
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| [(-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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],
]
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