-
Notifications
You must be signed in to change notification settings - Fork 110
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
Accommodate AbstractFFTs.jl#26 fftfreq etc. #300
Conversation
Could you also:
|
Agreed there should be deprecation warnings here, though I don't think they should guide the user to use I guess people writing generic code that takes in arrays of unknown types would use Perhaps @ararslan or @stevengj can clarify what the expectation is for how the FFT packages are supposed to be used? |
From the AbstractFFTs README:
So I agree that deprecations shouldn't point directly to AbstractFFTs. |
@galenlynch I implemented you suggestions. Regarding the deprecation I just |
@crstnbr could you please implement @martinholters suggestions so we can merge this once a new tagged version on AbstractFFTs.jl is available? |
@galenlynch I'm currently on vacation. Will try to get back to this next week! |
Alright, tests pass locally (with |
Now that JuliaMath/AbstractFFTs.jl#26 has been merged and a new release has been tagged (https://github.com/JuliaMath/AbstractFFTs.jl/releases/tag/v0.5.0) this should be merged. |
This should set |
Why? DSP doesnt have AbstractFFTs as a direct dependency. So this shouldn't be necessary, right? Instead I believe we should set the FFTW compat, which I do in #321. |
We just need to make sure that DSP can't get AbstractFFTs < 0.5. |
Something like JuliaMath/FFTW.jl#124 will be necessary, I think. (cf #321 (comment)) |
@@ -6,6 +6,7 @@ using ..Util, ..Windows | |||
export arraysplit, nextfastfft, periodogram, welch_pgram, mt_pgram, | |||
spectrogram, power, freq, stft | |||
using FFTW | |||
import FFTW: Frequencies, fftfreq, rfftfreq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import FFTW: Frequencies, fftfreq, rfftfreq |
I believe this isn't necessary, since there's a using FFTW
and FFTW reexports the symbols from AbstractFFTs. Same with the import from DSP in the tests.
This PR accommodates JuliaMath/AbstractFFTs.jl#26.
The definitions for
fftfreq
,rfftfreq
, as well as theFrequencies
type has been moved to AbstractFFTs.jl. Hence these definitions aren't necessary in DSP.jl anymore.All tests pass locally. They will fail here, since JuliaMath/AbstractFFTs.jl#26 hasn't been merged yet.
Do not merge before JuliaMath/AbstractFFTs.jl#26 has been merged!