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

add transpose method to _Audio #481

Merged
merged 17 commits into from Dec 15, 2023
Merged

Conversation

artpelling
Copy link
Contributor

@artpelling artpelling commented Jul 28, 2023

This PR adds a transpose method to all Signals, very similar to np.ndarray.transpose.

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 PR. Only a few comments on the docstring and some gneral points to consider/discuss

  • If we specify axes for audio objects we have the concept of ignoring the last axis holding the time frequency bins. This means, that axis -1 in an audio object refers to axis -2 of the underlying numpy array _data. You can account for this by axes = [a-1 if a<0 else a for a in axes] and add a description for this in the docstring.
  • I noted that the test are failing. Might be because you branched main. Can you branch develop instead?
  • Once the code is approved we should add tests to tests/test_audio_signal.py, tests/test_audio_time_data.py, and tests/test_audio_frequency_data.py

pyfar/classes/audio.py Show resolved Hide resolved
pyfar/classes/audio.py Outdated Show resolved Hide resolved
pyfar/classes/audio.py Outdated Show resolved Hide resolved
pyfar/classes/audio.py Outdated Show resolved Hide resolved
pyfar/classes/audio.py Outdated Show resolved Hide resolved
@artpelling artpelling changed the base branch from main to develop August 3, 2023 09:42
@artpelling
Copy link
Contributor Author

I've implemented your suggestions. Out of lazyness, I removed all assertions and simply transpose a mock array with identical ndim before deep copying instead of reverse engineering all rules. This will throw numpy's exceptions. Let me know if you like it or not.

  • Once the code is approved we should add tests to tests/test_audio_signal.py, tests/test_audio_time_data.py, and tests/test_audio_frequency_data.py

Will do that once the code is agreed on.

Copy link
Member

@ahms5 ahms5 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 PR, very elegant, looks fine to me. for me just tests are remaining. sth for flake8 also failed besides #483

@artpelling
Copy link
Contributor Author

@f-brinkmann This might be ready now.

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, looks very good already. I only had minor comments on the docstring and testing left. Can you also add a brief test for the shorthand signal.T?

pyfar/classes/audio.py Outdated Show resolved Hide resolved
pyfar/classes/audio.py Outdated Show resolved Hide resolved
tests/test_audio_frequency_data.py Show resolved Hide resolved
tests/test_audio_frequency_data.py Outdated Show resolved Hide resolved
@f-brinkmann f-brinkmann added v0.7.0 audio Classes pyfar.classes.audio labels Oct 13, 2023
artpelling and others added 2 commits October 26, 2023 21:02
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
@f-brinkmann f-brinkmann self-requested a review October 29, 2023 12:53
f-brinkmann
f-brinkmann previously approved these changes Oct 29, 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.

There seems to be an error in the tests now, but otherwise approved. Thanks for the effort.

@ahms5 ahms5 self-requested a review December 8, 2023 12:11
@f-brinkmann f-brinkmann merged commit f3d2645 into pyfar:develop Dec 15, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio Classes pyfar.classes.audio v0.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants