-
Notifications
You must be signed in to change notification settings - Fork 13
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
Test find impulse response delay complex #573
Test find impulse response delay complex #573
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.
I have just the confusion about the 'psd'
for normalization, otherwise its fine.
pyfar/classes/audio.py
Outdated
if self._complex and fft_norm == "rms": | ||
raise ValueError(("'rms' normalization is not valid for " | ||
"complex time signals")) | ||
if self._complex and fft_norm in ["rms", "power"]: |
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 'psd'
?
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.
This is part of #567 - should be fixe after that one is merged...
pyfar/classes/audio.py
Outdated
@@ -957,8 +957,8 @@ def fft_norm(self, value): | |||
raise ValueError(("Invalid FFT normalization. Has to be " | |||
f"{', '.join(self._VALID_FFT_NORMS)}, but found " | |||
f"'{value}'")) | |||
if self._complex and value == "rms": | |||
raise ValueError(("'rms' normalization is not valid for " | |||
if self._complex and value in ["rms", "power"]: |
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 'psd'
?
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.
see above
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.
Are all of those changes required for the delay estimation? There might be other changes in this PR.
pyfar/dsp/dsp.py
Outdated
for ch in np.ndindex(impulse_response.cshape): | ||
# Calculate the correlation between the impulse response and its | ||
# minimum phase equivalent. This requires a minimum phase equivalent | ||
# in the strict sense, instead of the appriximation implemented in | ||
# pyfar. | ||
n_samples = impulse_response.n_samples | ||
for idx, mode in zip(range(0, len(modes)), modes): |
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.
This looks better
for idx, mode in zip(range(0, len(modes)), modes): | |
for idx, mode in enumerate(modes): |
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, your're right.
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 we need all these changes, see the discussion in slack:
We want to calculate the delay independently for real and imaginary part, and return the minimum of both. I don't see a more elegant way.
Which issue(s) are closed by this pull request?
Closes #
Changes proposed in this pull request: