-
Notifications
You must be signed in to change notification settings - Fork 32
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
Move Frequencies, fftfreq, rfftfreq from DSP.jl to AbstractFFTs.jl #26
Conversation
Note that a quantity from Unitful.jl doesn't currently work due to julia> u = 3 * Unitful.Hz
3 Hz
julia> u isa Real
false as has been indicated here. Should we generalize to (I decided to make the change until someone complains.) |
So now a number with both physical units and uncertainty should work, right? 😃 |
Yes. julia> fftfreq(4, (5 ± 1) * Unitful.Hz)
4-element Frequencies{Quantity{Measurement{Float64},�^-1,Unitful.FreeUnits{(Hz,),�^-1,nothing}}}:
0.0 ± 0.0 Hz
1.25 ± 0.25 Hz
-2.5 ± 0.5 Hz
-1.25 ± 0.25 Hz |
Maybe the optional sampling frequency should rather be a keyword argument (in the spirit of #15)? Otherwise this should be good to go. |
Oh, I see the reason for assigning the Nyquist element (the middle frequency for even Of course, it's completely arbitrary whether we call this frequency positive or negative, but it's annoying that it is inconsistent with the real-input FFT case where we call all the frequencies nonnegative. But I guess we can't change the |
Ready for merge? |
It might be worth thinking about how this change affects other packages. E.g. how will this affect DSP.jl, where this code was taken from, and do other packages depend on these functions being in DSP.jl? It would be great if you could also open PRs against other affected packages to accommodate this change. |
Well, this PR is only adding a new feature. Therefore, technically, no other package is affected - the PR is trivially non-breaking. Maybe DSP.jl is an exception because I took the code from there and we probably want to avoid multiple identical definitions in different packages. I'll create a PR over there to switch to using AbstractFFTs If you're suggesting to make PRs against all packages that use DSP.jl for the sole purpose of getting |
DSP.jl PR created: JuliaDSP/DSP.jl#300 However, it can only be incorporated after this PR here has been merged. |
Maybe it would be enough to have DSP.jl still export |
Is it possible to have a new release of AbstractFFTs with this PR? |
Just need a PR to update Project.toml, I think? |
@stevengj Could you please tag a new version so we can fix the consequences of this in DSP.jl (JuliaDSP/DSP.jl#306)? |
This is another attempt at JuliaRegistries#5230, which was an attempt at fixing the breakage caused by JuliaMath/AbstractFFTs.jl#26 (e.g. JuliaDSP/DSP.jl#320, JuliaDSP/DSP.jl#323, JuliaDSP/DSP.jl#324). The underlying problem is that JuliaMath/AbstractFFTs.jl#26 moved functions from DSP.jl to AbstractFFTs.jl, requiring users to have versions of both of those packages either before that change or after it. Because FFTW reexports AbstractFFTWs, users also have to coordinate versions of DSP.jl and FFTW.jl. This problem was fixed for the most recent versions of DSP.jl and FFTW.jl (v0.6.1 and v1.1, respectively) by JuliaDSP/DSP.jl#322 and JuliaMath/FFTW.jl#124. However, because earlier versions of FFTW do not have an upper bound for its dependency on AbstractFFTs, and neither does DSP, Pkg cannot currently tell that some combinations of versions for AsbtractFFTs and DSP.jl are incompatible, which is causing users to experience issues even after the problem was fixed for the most recent versions (JuliaDSP/DSP.jl#323, JuliaDSP/DSP.jl#324). Fixing the problem is made slightly more complicated because DSP does not explicitly depend on AbstractFFTs, but instead DSP implicitly depends on AbstractFFTs through FFTW's reexport of it, and in turn DSP explicitly depends on FFTW. To fix this problem, I have changed the Compat.toml for FFTW to place a upper bound on AbstractFFTs, so that packages get a consistent interface when using FFTW. I have also changed the Compat.toml for DSP, to avoid breakage caused by AbstractFFTs v0.5 taking some functions that were previously defined in DSP. In addition, the Compat.toml for DSP showed versions before 0.5.1 to be compatible with Julia 1, which was inaccurate. I have therefore changed this TOML table to restrict the DSP versions to the range that is compatible with Julia 1. I have tested that these changes work for the most recent versions of DSP and FFTW, as well as older versions.
This is another attempt at #5230, which was an attempt at fixing the breakage caused by JuliaMath/AbstractFFTs.jl#26 (e.g. JuliaDSP/DSP.jl#320, JuliaDSP/DSP.jl#323, JuliaDSP/DSP.jl#324). The underlying problem is that JuliaMath/AbstractFFTs.jl#26 moved functions from DSP.jl to AbstractFFTs.jl, requiring users to have versions of both of those packages either before that change or after it. Because FFTW reexports AbstractFFTWs, users also have to coordinate versions of DSP.jl and FFTW.jl. This problem was fixed for the most recent versions of DSP.jl and FFTW.jl (v0.6.1 and v1.1, respectively) by JuliaDSP/DSP.jl#322 and JuliaMath/FFTW.jl#124. However, because earlier versions of FFTW do not have an upper bound for its dependency on AbstractFFTs, and neither does DSP, Pkg cannot currently tell that some combinations of versions for AsbtractFFTs and DSP.jl are incompatible, which is causing users to experience issues even after the problem was fixed for the most recent versions (JuliaDSP/DSP.jl#323, JuliaDSP/DSP.jl#324). Fixing the problem is made slightly more complicated because DSP does not explicitly depend on AbstractFFTs, but instead DSP implicitly depends on AbstractFFTs through FFTW's reexport of it, and in turn DSP explicitly depends on FFTW. To fix this problem, I have changed the Compat.toml for FFTW to place a upper bound on AbstractFFTs, so that packages get a consistent interface when using FFTW. I have also changed the Compat.toml for DSP, to avoid breakage caused by AbstractFFTs v0.5 taking some functions that were previously defined in DSP. In addition, the Compat.toml for DSP showed versions before 0.5.1 to be compatible with Julia 1, which was inaccurate. I have therefore changed this TOML table to restrict the DSP versions to the range that is compatible with Julia 1. I have tested that these changes work for the most recent versions of DSP and FFTW, as well as older versions.
See discussions in #19 and JuliaDSP/DSP.jl#298.
I have basically copy pasted the relevant code from DSP.jl and modified it according to @stevengj's comment.
This way one can use, for example, a measurement as sampling frequency:
Closes #19.