Skip to content

Conversation

@al-de
Copy link

@al-de al-de commented Mar 17, 2021

The classes 'Device' (to hold data of a single device in the measurement chain) and 'MeasurementChain' (to represent the complete measurement chain, including no, one or many devices) were implemented as part of the seminar 'Python for Acoustics and Audio Signal Processing' at TU Berlin.

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.

Looks good to me. Only one question: Do you check if the sampling rates of the Devices agree when you init a MeasurementChain with a list of Devices? I did not find that, but it should be checked in MeasurementChain.init

Comment on lines +195 to +197
# check if device_data is type Signal or None
if not isinstance(data, (Signal, type(None))):
raise TypeError('Input data must be type Signal or None.')
Copy link
Member

Choose a reason for hiding this comment

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

This is also checked in Device and could be removed here

Copy link
Author

Choose a reason for hiding this comment

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

When removing this check and passing something else then None or a Signal as data to add_device, the sampling-rate-check if not self.sampling_rate == data.sampling_rate: would raise an AttributeError, as the data has no attribute 'sampling_rate'. My thought was to raise a more meaningful error, but I'm not sure whether this is the best solution.

Comment on lines 42 to 46
time = [1, 0, 0, 0, 0, 0, 0, 0]
freq = [1, 1, 1, 1]
sampling_rate = 44100
fft_norm = 'none'
signal = signal_stub(time, freq, sampling_rate, fft_norm)
Copy link
Member

Choose a reason for hiding this comment

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

If you put this as a ficture in conftest.py you can reuse it :)
You can have a look at pyfar to see how this works...

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, I added a conftest.py and defined a flat_freq fixture in my latest commit. :)

al-de and others added 9 commits March 27, 2021 17:11
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
…surementChain

Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
@al-de
Copy link
Author

al-de commented Mar 27, 2021

Looks good to me. Only one question: Do you check if the sampling rates of the Devices agree when you init a MeasurementChain with a list of Devices? I did not find that, but it should be checked in MeasurementChain.init

Thanks a lot for your feedback. I just implemented all of your suggestions for mestopy.
With your question you were totally right, this check was missing in my code. I solved this with my latest commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants