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
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 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 byaxes = [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
d25107a
to
387f9b9
Compare
I've implemented your suggestions. Out of lazyness, I removed all assertions and simply transpose a mock array with identical
Will do that once the code is agreed on. |
3ed110e
to
d6748ee
Compare
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 PR, very elegant, looks fine to me. for me just tests are remaining. sth for flake8 also failed besides #483
8b310ea
to
384885f
Compare
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
384885f
to
389eadd
Compare
7304d94
to
2f40e87
Compare
2f40e87
to
fe4cf05
Compare
@f-brinkmann This might be ready now. |
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, 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?
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
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.
There seems to be an error in the tests now, but otherwise approved. Thanks for the effort.
This PR adds a
transpose
method to allSignals
, very similar tonp.ndarray.transpose
.