cwt with fft convolution support#490
Conversation
|
This is my first pull request on GitHub, please tell me if everything is in order |
|
Great, thanks @alsauve! I will take a look. The initial failures in the automated Windows testing on Appveyor are related to lack of |
|
Hi @grlee77, For this typing issue, after some digging I found in Before upgrading to a more robust type handling in cwt() I suggest to comment out for the time being the test function draft test_cwt_dtype() |
|
This looks pretty good. I will push some minor stylistic changes to your branch and then leave comments on any remaining issues.
Nice, I was not aware of that one. I would just delete |
grlee77
left a comment
There was a problem hiding this comment.
Congrats on starting your first PR!
I am in favor of the functionality added in this PR, but please see the review comments for some issues to consider.
|
Also, my changes in a767fc6 are mostly to code that predates this PR. They do not change any functionality, but are just things I noticed and updated a bit when making the other style fixes. The refactored code should be a bit more efficient, but it won't make a big difference because the computation time is dominated by the convolutions. |
… accordingly to #pullrequestreview-227464829
|
A merged version has been pushed with the comments taken into account. Also the next_fast_len() in cunjunction with caching of the fft of data buffer improved significantly the performances (see figures in change request), so it is included in this version. |
|
The appveyor build has failed due to the new requirement on module |
|
Yeah, we don't want to require scipy, but could make it an optional dependency and just fall back to the simple power of two solution when it is unavailable. (try/except ImportError) In the future it may be beneficial to allow use of the fft from scipy rather than numpy because we have plans on the scipy end to allow user configuration of the fft library used under the hood. Hopefully we will have a student working on that this summer as part of Google Summer of Code, but that has not been finalized yet. I would just keep using np.fft.fft for now. |
|
It used to be that NumPy and SciPy were both using FFTPack to do the FFTs. Very recently (for the upcoming 0.17 release), NumPy switched to pocketfft instead. Based on its readme: The SciPy function uses factors 2, 3 and 5 so it will still be applicable (although neglecting potential factors of 7 and 11). |
| def next_fast_len(n): | ||
| """Given a number of samples `n`, returns the next power of two | ||
| following this numbe to take advantage of FFT speedup. | ||
| This fallback is less efficient as `scipy.fftpack.next_fast_len` |
There was a problem hiding this comment.
minor typos:
numbe -> number
"less efficient as" -> "less efficient than"
There was a problem hiding this comment.
Hi, thanks for the typos, and btw, congrats for having taken your project to google summer camp.
I looked for pywavelet on the google camp site but could not find it.
In which section is it?
There was a problem hiding this comment.
The GSOC project I was referring to is for SciPy itself rather than PyWavelets. The various Python projects planning to participate in GSOC are listed at:
https://blogs.python-gsoc.org/en/project-ideas/
and the specific project I mentioned is the fftpack one described here:
https://github.com/scipy/scipy/wiki/GSoC-2019-project-ideas
At this point SciPy has received final proposals from potential students, but we won't know until May 6th whether our proposal has been accepted by Google.
|
BTW I made plots of a benchmark reference with a jupiter notebook here: The first run was somewhat biased by a cron job so I'll rerun rerunning the bench on my laptop. But the general idea is there, the graph helps understanding where the method change become interresting. |
Great. I will try to run that with base numpy (nomkl) and then one with (mkl). I think both variants are now available via conda-forge. We also have a benchmark suite for some things in PyWavelets, although we have not set up any mechanism to host the results online (I think NumPy does that for theirs). If you look in the There is a basic benchmark for cwt here: We could add a case to also loop over methods. If you want to try that we could add it in this PR or we can leave it to a future PR. If you do so, you have to test the benchmarks locally to make sure they run. They do not get automatically run by the CI services. |
|
Hi @alsauve: sorry this PR was neglected for some time. I think it looks pretty good. I updated the import logic a bit and added a cases running the FFT-based variant to the CWT benchmarks. Benchmark results (with SciPy 1.3, so using As expected, cases with larger size and larger number of scales show large benefit for the FFT-based approach. via |
|
I will probably have to rebase on current master to get the CI to pass here. |
|
Hi @grlee77, good to hear about you. I was myself pretty busy and also neglected to restart this thread. The times of your benchmark seem in line with my own checks here. I remember now that the last addition of scipy next_fast_len() had improved significantly the processing times. I consider this path pretty stable and usable as is, so you can proceed for the merge :-) |
|
@rgommers: Is there any objection to an optional import of I guess importing @alsauve: The rationale for preferring |
I agree, useful speedup. Would be good to add SciPy in one of the CI runs though. |
change 3.7-dev to 3.7 in .travis.yml
|
closes #479 |
|
Thanks @alsauve! |
This pull request contains the support for fast convolution support with fast fourier transform (FFT) as discussed in issue #479 .
The speed improvement is visible for signals over signal/scales over 100/50, and becomes highly significant by more than a factor 10 for EEG like signals with 100K+ samples.
From a user point of view the only change is the new keyword
method='fft', the behaviour of the function has been kept identical to the 1.0.2 release for both signature and dtype of output. Support for compliance between input and output dtype has been prepared.