Fixed #34 Customizing scipy's oaconvolve

[NimaSarajpoor]

This PR is to address #34.

[gitnotebooks]

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

[NimaSarajpoor]

./timing.py -timeout 1.0 -pmin 7 -pmax 24 pyfftw pocketfft_r2c_c2r scipy_oaconvolve challenger > timing.csv

# in timing.py, I change timeout to 5.0 when `len(T) >= 2^20`

The challenger is the customized version of scipy's oaconvolve.

Observations:

• The challenger outperforms scipy's oaconvolve
• For len(Q) <= 2 ^16 (and len(Q)>= 2^7), challenger outperforms the others for the most part.
• For len(Q) > 2^16, pocketfft outperforms the others for the most part.

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.

[seanlaw]

As a gentle reminder, even if we can do things faster, we will never (??) remove the public scipy convolution functions from STUMPY because they should be our last resort fallback (in case the alternatives, that may use private functions, raise an error). Does that make sense?

[NimaSarajpoor]

Good reminder. It makes sense!!

[NimaSarajpoor]

This line needs to be modified if, at a later time, we decide to move the proposal to a new file (module).

[NimaSarajpoor]

./timing.py -timeout 1.0 -pmin 7 -pmax 24 pyfftw pocketfft_r2c_c2r scipy_oaconvolve challenger > timing.csv

# in timing.py, I change timeout to 5.0 when `len(T) >= 2^20`

@seanlaw
"Challenger" seems to be the winner for most cases, and I think it is worth it to include it. What do you think? Also, can you please review the script? I've made major changes. The private objects are now only r2c and c2r. IMO, the script looks cleaner now.

[seanlaw]

@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

[NimaSarajpoor]

@seanlaw
I think this is ready for another round of review. Note that:

(1) I intentionally left a few things to be addressed in a different PR later for the function _compute_block_size. This can help us see the impact of those changes once the current PR is merged.

(2) I thought about refactoring '_pocketfft_circular_convolve_block' (in challenger_sdp.py) and '_pocketfft_valid_convolve' (in the changed file pocketfft_r2c_c2r_sdp.py). However, I couldn't find a clean way to do it. Maybe we need to create a new function and those both functions need to call that common function (??)

[seanlaw]

I think this is ready for another round of review

@NimaSarajpoor Was there anything in particular that you wanted me to focus on. Frankly, I don't have enough details or context to follow most/all the oaconvolve/block size logic. It feels like I can only attempt to spot issues with programming logic but not issues with algorithmic logic (not unless you can spell out the problem in clear detail).

Maybe we need to create a new function and those both functions need to call that common function (??)

I'll try to take a look but it's not clear to me what this is currently referencing as I do not have sufficient context

[NimaSarajpoor]

@seanlaw
Thanks for pointing that out. I would like to bring this to a point where it could be a good candidate for eventually being added to STUMPY/sdp.py. So, making both the algorithmic logic and the code easier to follow would be valuable.

Perhaps I should add some more context to this PR (?) For example, I can create a notebook that walks through the overlap-add approach mathematically and shows why, in our particular case, it produces the same result as the direct valid convolution.

Would something like that make the algorithmic logic easier to review? Or is there another kind of context that you think would be more useful?

[seanlaw]

@NimaSarajpoor Yes, I think a notebook would be valuable. In most/all of the matrix profile work, we nearly always had a published paper to reference (and their code) and so it was easier to start from the same shared context and then we could evolve from there together. Here, I'm purely following you and the context that you give me. So, I think a detailed notebook (even with naive code snippets) would be quite helpful especially to "see" where the hard problem(s) are that need to be solved