Skip to content
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

Fix welch_pgram!, update periodogram docs #557

Merged
merged 5 commits into from
May 12, 2024
Merged

Conversation

kcin96
Copy link
Contributor

@kcin96 kcin96 commented Apr 12, 2024

This PR

  1. Update docs to mention that power spectral densities are computed for periodograms as per Power, or power spectral density of Periodogram? #480
  2. Update periodogram docs with examples.
  3. Fixes welch_pgram! so Floats can work as well as Ints.
  4. Add test to welch_pgram!

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.74%. Comparing base (67d62c9) to head (374fc1d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #557   +/-   ##
=======================================
  Coverage   97.74%   97.74%           
=======================================
  Files          18       18           
  Lines        3197     3199    +2     
=======================================
+ Hits         3125     3127    +2     
  Misses         72       72           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wheeheee
Copy link
Member

Sorry, I don't really see how floats are not currently accepted as inputs to welch_pgram!. AFAICT your change relaxes the type constraints on s, which seems wrong to me since I feel if someone's using a config they should take care to match it to their input. Care to explain?

@kcin96
Copy link
Contributor Author

kcin96 commented Apr 26, 2024

Currently, when I run this block of code to test for Floats, I get a method error.

julia> Fs = 100;                     # Sampling frequency

julia> t = (1:Fs)/Fs;                # 100 time samples

julia> x = cos.(2π*25*t);            # Cosine signal with 25Hz frequency

julia> nfft = 512;

julia> y = vec(zeros(1, nfft));

julia> wconfig = WelchConfig(x; n=10, onesided=false, nfft=nfft, fs=Fs, window=hamming);

julia> pxx = welch_pgram!(y, x, wconfig);
ERROR: MethodError: no method matching welch_pgram!(::Vector{Float64}, ::Vector{Float64}, ::WelchConfig{Int64, AbstractFFTs.Frequencies{Float64}, Vector{Float64}, FFTW.rFFTWPlan{Float64, -1, false, 1, Tuple{Int64}}, Vector{Float64}, Vector{ComplexF64}, Float64})
Closest candidates are:
  welch_pgram!(::AbstractVector, ::AbstractVector, ::Int64) 
  welch_pgram!(::AbstractVector, ::AbstractVector, ::Int64, ::Int64; kwargs...) 
  welch_pgram!(::AbstractVector, ::AbstractVector{T}, ::WelchConfig{T}) where T<:Number
  ...
Stacktrace:
 [1] top-level scope
   @ REPL[24]:1

Whereas, if I next run the welch_pgram(s::AbstractVector, config::WelchConfig) function, I get a result.

julia> welch_pgram(x, wconfig)
DSP.Periodograms.Periodogram{Float64, AbstractFFTs.Frequencies{Float64}, Vector{Float64}}([2.656596353078254e-7, 2.8336305486805697e-7, 3.3679303813800914e-7, 4.269139053267828e-7, 5.553500731514562e-7, 7.244120931838832e-7, 9.371335271002283e-7, 1.1973190664269316e-6, 1.5096044010914354e-6, 1.8795284233756564e-6    2.313618418942876e-6, 1.8795284233756564e-6, 1.5096044010914354e-6, 1.1973190664269316e-6, 9.371335271002283e-7, 7.244120931838832e-7, 5.553500731514562e-7, 4.269139053267828e-7, 3.3679303813800914e-7, 2.8336305486805697e-7], [0.0, 0.1953125, 0.390625, 0.5859375, 0.78125, 0.9765625, 1.171875, 1.3671875, 1.5625, 1.7578125    -1.953125, -1.7578125, -1.5625, -1.3671875, -1.171875, -0.9765625, -0.78125, -0.5859375, -0.390625, -0.1953125])

Not too sure if this is the intent?

@wheeheee
Copy link
Member

wheeheee commented Apr 26, 2024

Oh right, yeah, seems like the type parameters are wrong, but I think the fix should also include another check that float(eltype(data)) == T1 or something along those lines

@kcin96
Copy link
Contributor Author

kcin96 commented Apr 26, 2024

That is a good point! So something like this which checks that the type of input s matches that of config.inbuf

function welch_pgram!(out::AbstractVector, s::AbstractVector, config::WelchConfig{T}) where T<:Number
     if length(out) != length(config.freq)
         throw(DimensionMismatch("""Expected `output` to be of length `length(config.freq)`;
             got `length(output)` = $(length(out)) and `length(config.freq)` = $(length(config.freq))"""))
    elseif eltype(out) != fftabs2type(T)
        throw(ArgumentError("Eltype of output ($(eltype(out))) doesn't match the expected "*
                            "type: $(fftabs2type(T))."))
    elseif eltype(s) != eltype(config.inbuf)
        throw(ArgumentError("Eltype of input `s` ($(eltype(s))) doesn't match the expected `config` "*
                            "type: $(eltype(config.inbuf))."))
end

@wheeheee
Copy link
Member

wheeheee commented Apr 26, 2024

No, the problem is also that the first type parameter of the WelchConfig is the eltype of fs, which isn't what should be checked. The T in this function should instead just be the type parameter in s::AbstractVector{T}. Then eltype(s) can be replaced by T etc

@kcin96
Copy link
Contributor Author

kcin96 commented Apr 26, 2024

I see, you're right, and it is much cleaner now.

@wheeheee wheeheee changed the title Update periodogram documentation. Fix welch_pgram!, update periodogram docs Apr 26, 2024
@wheeheee wheeheee linked an issue May 1, 2024 that may be closed by this pull request
@wheeheee wheeheee merged commit 76001f4 into JuliaDSP:master May 12, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Power, or power spectral density of Periodogram?
2 participants