-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add complex valued bandpass filter #468
Conversation
src/Filters/design.jl
Outdated
@@ -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)") |
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.
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.
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). |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
08c30a6
to
72a188a
Compare
9fd0547
to
5e30601
Compare
5e30601
to
9a2d9e6
Compare
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.
I think constructor should return values of the type they're asked to construct, which appears not to be the case here.
9a2d9e6
to
70119bf
Compare
70119bf
to
4941d9d
Compare
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... |
b7cb383
to
373f26f
Compare
LGTM modulo the minor docstring fix. Good to go then or are there any objections? |
|
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 |
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.
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.
1257934
to
f60f770
Compare
Is this good to go? Or maybe an additional commit changing the bounds for |
f5119f8
to
2b860b4
Compare
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.
LGTM.
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]>
2b860b4
to
a3e8066
Compare
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 forscale = 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: