Fixed #34 Customizing scipy's oaconvolve - #35
Conversation
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/sliding_dot_product/pull/35 |
The challenger is the customized version of scipy's oaconvolve. 
Observations:
For me, the important one is the first bullet point. Of the four optimization opportunities mentioned in this comment, I've addressed 1, 2, and 3 in this PR. The last item, which is about adjusting the number of multiplication for real-valued arrays, can be explored next. |
|
As a gentle reminder, even if we can do things faster, we will never (??) remove the public |
|
Good reminder. It makes sense!! |
|
|
||
|
|
||
| def test_oaconvolve_sdp_blocksize(): | ||
| from sdp.challenger_sdp import sliding_dot_product |
There was a problem hiding this comment.
This line needs to be modified if, at a later time, we decide to move the proposal to a new file (module).
@seanlaw |
There was a problem hiding this comment.
@NimaSarajpoor I've left some comments but would still like another pass after you've cleaned things up further
I do agree that, for the most part, things look clean. I think it still lacks clarity as to what is happening or why the logic is coded in this way
| if conv_block_size is None: | ||
| # Find optimal block_size based on m and n | ||
| if m >= n / 2: | ||
| conv_block_size = n # i.e. no blocking |
There was a problem hiding this comment.
Not clear why this condition is needed ?
There was a problem hiding this comment.
This is purely based on scipy's logic:
However, the reason is not clear to me. TBD
|
|
||
|
|
||
| def sliding_dot_product(Q, T): | ||
| def _pocketfft_valid_convolve(Q, T): |
There was a problem hiding this comment.
Add docstring:
"""
Compute the valid convolution between Q and T using circular convolution
"""
|
|
||
| Notes | ||
| ----- | ||
| The valid convolution between `Q` and `T` is computed by sliding Q[::-1] |
There was a problem hiding this comment.
The term "compute" might be slightly misleading here because the actual compute is not in time domain, and it is actually in frequency domain via circular convolution.
| conv_block_size = next_fast_len(math.ceil(opt_size), real=True) | ||
|
|
||
| # Ensure that conv_block_size is at least m, so that | ||
| # it can cover at least one element of `T` in each block |
There was a problem hiding this comment.
As I am reading this comment again, I can see this is not clear. How about this?
# Each chunk of `T` is padded with `m - 1` zeros to form a convolution block.
# Since a chunk (from `T`) must contain at least one element,
# the minimum block size is `m`.
| Block size for the convolution. Will be at least `m` and at most `n`. | ||
| """ | ||
| if conv_block_size is None: | ||
| if m >= n / 2: |
2 participants
This PR is to address #34.