-
Notifications
You must be signed in to change notification settings - Fork 296
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
Looping audio #136
Looping audio #136
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.
Have yet to add an sf.write()
for the stereo audio, slightly confused on how the implementation would be different from the mono version (which was the default)
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.
Have yet to add an sf.write() for the stereo audio, slightly confused on how the implementation would be different from the mono version (which was the default)
You can just call ret_and_save_audio
from augly.audio.utils
! It works for any number of channels & different audio formats.
still a WIP I think somehow the last commit overwrote all the previous edit history |
@zpapakipos @jbitton The |
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.
Overall this looks good to me! So sorry that you had to go through all those merge conflicts / essentially re-do the PR. Good news is that you're some nits away from getting your first AugLy PR landed!!!
That issue in the GitHub workflow shouldn't be your fault. Once you address these nits etc, let's see if the tests now pass / if it was a transient error. If not, @zpapakipos and I will look into it more -- it could be that one of our dependencies got a new update and now something isn't working.
@jbitton has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
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.
Overall LGTM! A couple teeny tiny nits (mostly just adding newlines) and we're done! Btw, I ran your code internally and it passes :)
Co-authored-by: Joanna Bitton <joanna.bitton@gmail.com>
Co-authored-by: Joanna Bitton <joanna.bitton@gmail.com>
Co-authored-by: Joanna Bitton <joanna.bitton@gmail.com>
Co-authored-by: Joanna Bitton <joanna.bitton@gmail.com>
@jbitton Got it! The new commits should be there now. |
Looks fantastic @iAdityaEmpire !!!!!! |
@jbitton has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@jbitton has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Related Issue
Fixes #129
Summary
<Please summarize what you are trying to achieve, what changes you made, and how they achieve the desired result.>
Preliminary loop augmentation for the audio modality, yet to add stereo
test_loop_audio.wave
Unit Tests
If your changes touch the
audio
module, please run all of theaudio
tests and paste the output here. Likewise forimage
,text
, &video
. If your changes could affect behavior in multiple modules, please run the tests for all potentially affected modules. If you are unsure of which modules might be affected by your changes, please just run all the unit tests.Audio
Image
Text
python -m unittest discover -s augly/tests/text_tests/ -p "*"
Video
python -m unittest discover -s augly/tests/video_tests/ -p "*"
All
python -m unittest discover -s augly/tests/ -p "*"
Other testing
If applicable, test your changes and paste the output here. For example, if your changes affect the requirements/installation, then test installing augly in a fresh conda env, then make sure you are able to import augly & run the unit test