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
adapt convolution for complex singals and implement testing #461
Conversation
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.
Thanks for the implementation. I have two general comments
- Can you add a one line docstrings to the tests, e.g., '''Test dsp.convolve with complex signals'''?
- Should we use signals for testing that actually have an imaginary part?
pyfar/dsp/dsp.py
Outdated
if res.dtype.kind == 'c': | ||
is_result_complex = True | ||
else: | ||
is_result_complex = False |
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.
Could be shortened
if res.dtype.kind == 'c': | |
is_result_complex = True | |
else: | |
is_result_complex = False | |
complex = True if res.dtype.kind == 'c' else False |
add docstrings to tests shorten query for complex convolution results
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.
Thanks for implementing, looks fine to me, but I just have on question.
pyfar/dsp/dsp.py
Outdated
@@ -1656,8 +1656,11 @@ def convolve(signal1, signal2, mode='full', method='overlap_add'): | |||
res[..., :n_min-1] += res[..., -n_min+1:] | |||
res = res[..., :n_max] | |||
|
|||
complex = True if res.dtype.kind == 'c' else False |
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.
why not to take it from signal1
and signal2
? and check if res.dtype.kind make sence?
complex = True if res.dtype.kind == 'c' else False | |
complex = signal1.complex | signal2.complex |
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.
Thanks for the effort. just one comment.
('full', np.array([[1, -0.5, 0.1, -0.35, -0.05, 0.01]], dtype='complex')), | ||
('cut', np.array([[1, -0.5, 0.1, -0.35]], dtype='complex')), | ||
('cyclic', np.array([[0.95, -0.49, 0.1, -0.35]], dtype='complex'))]) | ||
def test_convolve_mode_and_method_complex(method, mode, desired): |
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.
How about testing complex signals and also one complex and one real signal.
implement convolution test for one complex and one real valued signal
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.
Thanks for implementing!
adapt convolution and implement tests