Add pyfftw sdp#1132
Conversation
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1132 |
There was a problem hiding this comment.
@NimaSarajpoor I've left some comments for you to address. I think we can afford to be clearer since all of this pyfftw stuff will be hard to maintain. We should probably be as verbose (and add more comments cross referencing their docs as possible). I'll do another few passes after you've responded and made modifications. I think this pyfftw stuff needs to be crystal clear
|
@seanlaw |
| except ImportError: # pragma: no cover | ||
| PYFFTW_IS_AVAILABLE = False | ||
|
|
||
|
|
There was a problem hiding this comment.
Maybe add a comment here that says: "The name of any callable object that computes the sliding dot product should end with sliding_dot_product"
| pytest.skip("Skipping Test pyFFTW Not Installed") | ||
|
|
||
| # This test checks that the pyfftw_sdp can be initialized | ||
| # with multiple threads and longdouble data type |
There was a problem hiding this comment.
| # with multiple threads and longdouble data type | |
| # longdouble data type, and then called in a multithreaded mode. |
Note that the number of threads is not set when the instance is created
| if not sdp.PYFFTW_IS_AVAILABLE: # pragma: no cover | ||
| pytest.skip("Skipping Test pyFFTW Not Installed") | ||
|
|
||
| # This test checks that the pyfftw_sdp can be initialized |
There was a problem hiding this comment.
| # This test checks that the pyfftw_sdp can be initialized | |
| # This test checks that the pyfftw_sdp instantiated object | |
| can be called in a multithreaded mode. |
| pytest.skip("Skipping Test pyFFTW Not Installed") | ||
|
|
||
| # This test checks that the pyfftw_sdp can be initialized | ||
| # with multiple threads |
There was a problem hiding this comment.
| # with multiple threads |
|
|
||
| Parameters | ||
| ---------- | ||
| max_n : int, default=2**20 |
There was a problem hiding this comment.
| max_n : int, default=2**20 | |
| max_n : int, default 2**20 |
| real-valued array. A complex-valued array of size `1 + (max_n // 2)` | ||
| will also be preallocated. | ||
|
|
||
| real_dtype : str, default="float64" |
There was a problem hiding this comment.
| real_dtype : str, default="float64" | |
| real_dtype : str, default "float64" |
|
@NimaSarajpoor This might be a distraction but a (very, very small) part of me was wondering if we should be using a closure instead of a class: https://realpython.com/python-closure/ I don't know if it makes things easier/harder to read. Given all of the array allocations, maybe a class is the right thing to use. I just wanted to bring it to your attention since I'm not sure we're getting the full benefits of a class since:
Just something to consider. No pressure though. |
This is something that bothered me too but thought the reason for being bothered is just my lack of experience and, also, I didn't know/think about an alternative option.
Thanks for sharing this! I will go through this and try to modify the script. Let's see how it will look like. |
This is to address
PR 3described in #1118 (comment). Have copied the corresponding notes below: