Wrapper function for private 1st and 2nd order allpass implementations #571
Conversation
pyfar/dsp/filter/audiofilter.py
Outdated
|
|
||
| .. math:: A(s_n) = \\frac{1-\\frac{a_i}{\\omega_c} s_n+\\frac{b_i} | ||
| {\\omega_c^2} s_n^2}{1+\\frac{a_i}{\\omega_c} s_n | ||
| +\\frac{b_i}{\\omega_c^2} s_n^2} |
There was a problem hiding this comment.
I think we can write s in stead of s_n and add below \omega_c = 2 \pi f_c with the cut-off frequency f_c and s=\sigma + \mathrm{i} \omega
tests/test_filter.py
Outdated
| # We thus only test the functionality not the results | ||
|
|
||
| fc = 1000 | ||
| orders = [1, 2] |
There was a problem hiding this comment.
It would be cleaner to use pytest.parametrize here: @pytest.parametrize('order', (1, 2))` (actual syntax might be slightly different you can search for it in the pyfar code)
There was a problem hiding this comment.
You can also prove more arguments in parametrize. For example you do
@pytest.parametrize('order, group_delay, frequency', ([1, group_delay_1, frequency_1], [2, group_delay_2, frequency_2])) and use this to test if the group delay of the allpass is close to group_delay below frequency
There was a problem hiding this comment.
Thanks for the tip.
Now i'm testing the group delay fc for
And the difference between fc.
Hope that makes any sense :D
pyfar/dsp/filter/audiofilter.py
Outdated
| @@ -85,48 +96,33 @@ def allpass(signal, frequency, order, coefficients=None, sampling_rate=None): | |||
| # check if coefficients match filter order | |||
| if coefficients is not None and ( | |||
| (order == 1 and np.isscalar(coefficients) is False) | |||
| or (order == 2 and ( | |||
| type(coefficients) is not list or len(coefficients) != 2))): | |||
| or (order == 2 and (isinstance(coefficients, (list, np.ndarray)) | |||
There was a problem hiding this comment.
using not isinstance(...) instead of isinstance(...) is False is more common and easier to reade
tests/test_filter.py
Outdated
| freqs = sig_filt.frequencies | ||
| idx = np.argmin(abs(freqs - fc)) |
There was a problem hiding this comment.
The Signal class already has a find_nearest_frequency method that you can use here :)
tests/test_filter.py
Outdated
|
|
||
| def test_allpass_warnings(impulse, fc=1000): | ||
| # test ValueError | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
We usually test if the expected error is thrown using the match. Can you add this here and below?
| with pytest.raises(ValueError): | |
| with pytest.raises(ValueError, match='some text'): |
tests/test_filter.py
Outdated
| # test ValueError | ||
| with pytest.raises(ValueError): | ||
| # pass signal and sampling rate | ||
| x = pfilt.allpass(impulse, fc, 1, sampling_rate=44100) | ||
| _ = pfilt.allpass(impulse, fc, 1, sampling_rate=44100) |
There was a problem hiding this comment.
I think you can simplfiy this here and below:
| _ = pfilt.allpass(impulse, fc, 1, sampling_rate=44100) | |
| pfilt.allpass(impulse, fc, 1, sampling_rate=44100) |
Remove real part from laplace variable in docstring Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com>
Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com>
Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com>
f-brinkmann
dismissed stale reviews from ahms5 and themself
The base branch was changed.
Wrapper function for allpass implementation #527
I probably should add a test for the filter properties to
test_filter.py. Any ideas for a smart way of checking if the filter is doing what it's supposed to do?Link to docs:
https://pyfar--571.org.readthedocs.build/en/571/modules/pyfar.dsp.filter.html#pyfar.dsp.filter.allpass