-
Notifications
You must be signed in to change notification settings - Fork 83
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
Drastically speed up spectrums by zero-padding to a power-of-two. #6
base: master
Are you sure you want to change the base?
Conversation
This should be the numpy default, IMO, but what can you do?
..wait. This causes |
SciPy uses FFTPACK which is optimized for multiples of 2, 3, 5, which I made a function for here: The function is _next_regular https://github.com/endolith/scipy/blob/master/scipy/signal/signaltools.py#L246 but planning on changing it to _next_opt_len in the future: https://github.com/endolith/scipy/blob/czt/scipy/fftpack/helper.py#L49 |
Oh great. So this will this end up in In the meantime, I don't suppose you have a clue why the padding I added is breaking your |
Oh, or are you saying that I should use |
No, but czt can replace prime-length ffts in the future
Yes, instead of powers of 2.
What are you expecting to get and what are you actually getting? |
…ound2() Depending on the BLAS you have there are myriad optimal sizes. [For example](scipy/scipy#3144 (comment)) the optimal length under one implementation is any of 2^a 3^b 5^c 7^d 11^e 13^f, where e+f is either 0 or 1, and the other exponents are arbitrary. It's just a matter of how the implementation chooses to carve up subproblems. Halves---making powers of 2 fast---is traditional, but not the only choice.
You know what? It was my sample. I was going to make a before and after test case and post it here, but as I waited and waited for the code to run across the vast majority of my corpus, I found that using a faster size or not only changes how the mistakes are made, not where. The mistakes almost always give different pitches, but the files that give mistakes trouble tend to be the same (there's 13 the unmolested version gets wrong where the Thanks again for your code. I've updated the PR as you requested. |
actually I'd be happier if next_regular were just copied into common.py as a public function instead of importing a private name that only exists in certain scipy versions and won't exist in future versions. |
Now it's a public function |
I don't think this code will work as written. Have you tested it before and after with known frequencies? The new FFT length will be I am trying to clean up the project so I can't test this yet |
6def0df
to
37ccd72
Compare
Thank you for this code. It is very helpful to have all the methods in estimate_frequency.py laid out and contrasted.
I'm not totally sure about the change to thd.