Skip to content
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 various dsp methods #525

Merged
merged 17 commits into from
Feb 23, 2024
Merged

Test various dsp methods #525

merged 17 commits into from
Feb 23, 2024

Conversation

tluebeck
Copy link

Which issue(s) are closed by this pull request?

Closes #

Changes proposed in this pull request:

  • test regularized spectrum inversion
  • test phase
  • test time shift
  • test window
  • test decibel
  • test resampling
  • test pad_zeros
  • test spectogram

tluebeck added 7 commits May 3, 2023 09:58
Unit tests for dsp methods that do not need to be adapted for complex signals
adapt decibel, time_shift, and pad_zero tests
add a complex plus impulse fixture, implement phase test for complex signals
@tluebeck tluebeck changed the base branch from main to develop_complex_signals December 17, 2023 04:44
@ahms5 ahms5 added the dsp label Dec 18, 2023
Copy link
Member

@f-brinkmann f-brinkmann left a 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. Only had one thing to change and a couple of optional suggestions.

20*np.log10(np.abs(test_Signal.freq)))
npt.assert_equal(pf.dsp.decibel(test_Signal, domain='freq_raw'),
20*np.log10(np.abs(test_Signal.freq_raw)))
# Testing overwrite log_prefix Parameter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be possible to remove all tests below, since they are already tested above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But not for complex signals, or do I miss something here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it might not be necessary to test all options of the function with complex signals. For example it should be enough testing the log_prefix with one signal. It's also fine to have it in there, but it might save time for further pulls to have an eye on this.

# check if results if still complex
assert padded.complex is True

desired = pyfar.signals.impulse(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be possible to remove all tests below since they are already tested above.

freqs, times, spectro = pf.dsp.spectrogram(signal, window='rect')

# check frequencies and times
npt.assert_allclose(freqs,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to re-arange the data in spectro and freq using np.fft.fftshift to force zero Hz to be in the center of the spectrum. You coud than assert if freqs equal signal.frequencies. This would be more consistent.

Copy link
Member

@f-brinkmann f-brinkmann left a 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. I only had one question in the average function

pyfar/dsp/dsp.py Outdated
@@ -1985,6 +1995,14 @@ def average(signal, mode='linear', caxis=None, weights=None, keepdims=False,
f"mode is '{mode}' and signal is type '{signal.__class__}'"
" but must be of type 'Signal' or 'FrequencyData'."))

if ((type(signal) is pyfar.TimeData or type(signal) is pyfar.Signal)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this for TimeData, but why are the modes not defined for complex Signals?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that. At the very beginning we said it. But with the last discussion this mode restriction is not necessary anymore. I removed it

@tluebeck tluebeck merged commit 02c6eb8 into develop_complex_signals Feb 23, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants