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

Add complex valued bandpass filter #468

Merged
merged 11 commits into from
Nov 5, 2024

Conversation

zsoerenm
Copy link
Contributor

This pull request allows to create ComplexBandpass filters for one sided pass bands (different pass bands for positive or negative frequencies)
I haven't implemented scalefactor(coefs::Vector, ftype::ComplexBandpass) yet, as I'm not really sure how to implement it for this case. So this currently only works for scale = false.
Also I haven't implemented tests, yet. Where do all the filter coefficients in the tests for the different filters come from? Maybe someone can help me out with this.
Here is an example:

using DSP, FFTW, Plots
signal = randn(ComplexF64, 10000)
filtered_signal = filt(digitalfilter(ComplexBandpass(-5e6, -3e6; fs = 10e6), FIRWindow(hanning(512), scale = false)), signal)
p = periodogram(filtered_signal, fs = 10e6)
Plots.plot(fftshift(freq(p)), 10 * log10.(fftshift(power(p))))

ComplexBandpass

@@ -241,6 +241,11 @@ function normalize_freq(w::Real, fs::Real)
f >= 1 && error("frequencies must be less than the Nyquist frequency $(fs/2)")
f
end
function normalize_complex_freq(w::Real, fs::Real)
f = 2*w/fs
f >= 2 && error("frequencies must be less than the Nyquist frequency $(fs)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also put a lower bound on f? It seems that 0 <= f < 2 and -1 <= f < 1 would be reasonable limits, but depending on context, one or the other may be more useful. So require -1 <= f < 2? Looks rather arbitrary. Or maybe we put no restriction on f at all? Really, I don't know.

@martinholters
Copy link
Member

Regarding tests, I think most of the reference data has been produced using comparable Matlab functions. Personally, I'm not too huge a fan of this. For the case at hand, I could imagine determining the frequency responses of a couple of designed filters and verifying that they fulfill the specifications (under some assumptions on what passband ripple and stopband gain can be reasonably expected).

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.84%. Comparing base (7652e2d) to head (a3e8066).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
+ Coverage   97.83%   97.84%   +0.01%     
==========================================
  Files          19       19              
  Lines        3226     3252      +26     
==========================================
+ Hits         3156     3182      +26     
  Misses         70       70              

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

@wheeheee wheeheee added this to the 0.8 milestone Feb 8, 2024
Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think constructor should return values of the type they're asked to construct, which appears not to be the case here.

src/Filters/design.jl Outdated Show resolved Hide resolved
@wheeheee
Copy link
Contributor

I think this PR just needs a reference signal for the test, probably written with scipy (replacing the placeholder one I left). Not sure how the complex bandpass is supposed to work, so I'm unable to do that...

@wheeheee wheeheee force-pushed the ss/complex-bandpass-filter branch from b7cb383 to 373f26f Compare May 14, 2024 20:30
@martinholters
Copy link
Member

LGTM modulo the minor docstring fix. Good to go then or are there any objections?

@wheeheee
Copy link
Contributor

scalefactor isn't implemented for complex bandpass. Is that meaningful or is it ok as is now?

@martinholters
Copy link
Member

Ah, good point. I think this should work:

function scalefactor(coefs::Vector{T}, ftype::ComplexBandpass, fs::Real) where T<:Number
    n = length(coefs)
    freq = normalize_complex_freq(middle(ftype.w1, ftype.w2), fs)
    c = zero(T)
    for k = 1:n
        c = muladd(coefs[k], cispi(-freq * (k - (n + 1) / 2)), c)
    end
    return abs(c)
end

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to re-create the example from the OP, I got different results which lead me to what I suspect to be a bug in how fs is used.

src/Filters/design.jl Outdated Show resolved Hide resolved
test/filter_design.jl Outdated Show resolved Hide resolved
@wheeheee
Copy link
Contributor

Is this good to go? Or maybe an additional commit changing the bounds for f?

@wheeheee wheeheee force-pushed the ss/complex-bandpass-filter branch 2 times, most recently from f5119f8 to 2b860b4 Compare November 5, 2024 07:48
Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

martinholters and others added 5 commits November 5, 2024 18:09
These tests differ from those of the other `digitalfilter` types in that
they don't compare to a reference impulse response, but rather check the
frequency response for plausibility.
Co-authored-by: Martin Holters <[email protected]>
@martinholters martinholters merged commit 3ed388e into JuliaDSP:master Nov 5, 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.

3 participants