Enable Analog Filter Design #458
Conversation
…er design When designing a digital filter using function digitalfilter() the sampling frequency is not passed to the FilterType definition anymore, but to function digitalfilter(). That means frequency normalization is not done during construction of FilterType anymore, but during filter design inside function digitalfilter(). For this the sampling frequency has to be passed to the internal functions prewarp(), firprototype() and scalefactor() as well. The intended side effect is, that an analog filter can now be designed by specifying analog frequencies directly without any frequency normalization and without a sampling frequency.
|
Haven't reviewed the code in detail, but conceptually, this makes sense to be. Downside is that this would be breaking. We're still at an 0.x version number to be able to do precisely such changes, but we should still be careful. Are we otherwise happy with the filter design API or should we take the opportunity and do more redesign work? |
I was also thinking about introducing domain specifiers |
Extend test case 'digital IIR' to test digital filter design with non-normalized frequency values in the filter response definition and specified sampling frequency.
|
I had a look on the test cases in test/filter_design.jl now. I think the test cases should be extended as follows:
I can find the following test cases for analog filter design:
Should these test cases be changed to use arbitrary (non-normalized) frequency values or should we add new test cases? All test cases are based on a MATLAB reference implementation. I don't have MATLAB, but could use Octave instead. Accuracy seems to be sufficient as well. Is Octave a problem? For digital filter design I can find the following test case:
We could use the existing reference implementation and simply add some code lines to design the same digital filter using non-normalized frequency values. I've provided a proposal (0d05b11). Or should we add new test cases? What do you think? |
|
Off topic: On my PC the test case Does anybody know, what could be the problem? I don't really understand function |
|
@martinholters Did you have the time to take a look at the code and the tests? |
|
Makes sense to me and from an admittedly not-yet-too-close look at the code and tests, everything looks good, but we should probably add tests for what this is solving: analog filters with frequencies outside the "normalized" range. |
|
I updated analog filter design tests now using non-normalized frequency values in the filter response definition. Unfortunately Octave's elliptic filter design function Additionally I fixed the organization and naming of the analog filter design test set. |
This is an amendment to commit 54bc9d5 where test sets have been introduced. The code following the current test set "freq. scaling" is actually a "lowpass" test. It seems that a corresponding comment on this low pass filter test was already missing before this commit. And the comment "Freqency scaling" seems to be meant for all four following test sets "lowpass", "highpass", "bandpass" and "bandstop".
The new test set name relates to the name of the tested function `analogfilter()` and not to what it is doing.
In the current version the function `analogfilter()` already generates a `ZeroPoleGain` filter representation. It seems, that in earlier versions this wasn't the case (see e.g. commit 7fd2102)
For providing new test data I had no MATLAB available and therefore switched to Octave. Unfortunatelly Octave's elliptic filter design function `ellip()` generates different filter coefficients than Julia when designing a 20th order analog filter. Maybe this is a bug in Octave's 'signal' package. I changed the test to design a Chebyshev type II filter instead.
gruberchr
force-pushed
the
analog_filter_design_#341
branch
from
e0f2141 to
a899b41
Compare
|
@martinholters Any progress on this MR? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #458 +/- ##
=======================================
Coverage 97.46% 97.46%
=======================================
Files 18 18
Lines 3080 3080
=======================================
Hits 3002 3002
Misses 78 78 ☔ View full report in Codecov by Sentry. |
|
I know this was reviewed a long time ago - but wondering if it is good to pull in and merge? |
martinholters
left a comment
martinholters
left a comment
There was a problem hiding this comment.
Yes, let's go ahead with this.
As it is a breaking change, we might want to bump the version number to 0.8.0-dev as part of this PR so that we don't forget. Opinions? @ViralBShah?
|
Yes, makes sense to go to 0.8-dev. |
|
Now that #532 bumped the version to 0.8, let's try this. |
|
Thanks @gruberchr and sorry for the long delay. |
3 participants
This is a proposal for fixing analog filter design (see bug #341). For this frequency normalization has been shifted to function
digitalfilter(). And functionanalogfilter()doesn't require any sampling frequency now. All affected tests have been adapted by simply providing sampling frequency to functiondigitalfilter()instead ofFilterTypeconstruction.Documentation has not been changed yet, I'll do this next. Additionally some new tests should be appended for analog filter design, I guess.
Any remarks?