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

Adding some audio transforms and augmentations to tonic #273

Merged
merged 42 commits into from
May 15, 2024

Conversation

MinaKh
Copy link
Contributor

@MinaKh MinaKh commented Nov 24, 2023

This branch includes following updates in tonic/develop:

  • Three transforms are added to audio_transforms.py:

    • SwapAxes, AmplitudeScale and robustAmplitudeScale
  • A new script is added: audio_augmentations.py containing wrapper classes for following audio augmentations:

    • RandomAmplitudeScale
    • RandomPitchShift
    • RandomTimeStreatch
    • RIR: adding room impulse response (echo effect)
    • AddWhiteNoise
  • corresponding tests added: test_audio_trnasform.py

  • A new script added for testing audio augmentations ---> test_audio_augmentations.py

  • A tutorial notebook is added in docs/tutorials/

@MinaKh MinaKh marked this pull request as draft November 24, 2023 12:40
@MinaKh MinaKh marked this pull request as ready for review November 24, 2023 12:43
@MinaKh MinaKh marked this pull request as draft November 24, 2023 14:39
@fabrizio-ottati
Copy link
Collaborator

@MinaKh I think you should add torchaudio to the test requirements.txt, 'cause it fails as dependency in the CI tests.

@MinaKh
Copy link
Contributor Author

MinaKh commented Nov 24, 2023

@MinaKh I think you should add torchaudio to the test requirements.txt, 'cause it fails as dependency in the CI tests.

Thanks, just did!

@fabrizio-ottati
Copy link
Collaborator

It seems that the tests now are running using CUDA. Maybe it is better to run them with everything on CPU? Or we could modify the requirements.txt to install the GPU version of PyTorch (both vision and audio).

@MinaKh
Copy link
Contributor Author

MinaKh commented Nov 24, 2023

It seems that the tests now are running using CUDA. Maybe it is better to run them with everything on CPU? Or we could modify the requirements.txt to install the GPU version of PyTorch (both vision and audio).

@fabrizio-ottati No, I can set them to run on cpu. Also there are other tests that I passed on my machine but fail on CI. I need to fix without need to install extra packages. Thanks!

Copy link
Member

@biphasic biphasic left a comment

Choose a reason for hiding this comment

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

Most transforms are encapsulated enough so that I can add them, but some stuff that uses QUTNoise things I won't be able to merge like that, unless it is made a bit more general. For example, maybe Tonic has a AddNoise class, but then in your user code at SynSense you call it with AddNoise(QUTNoise), after the principle of dependency injection

import torch
import torchaudio
import torchaudio.functional as F
from qut_noise import QUTNoise
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 a dependency that I won't be able to add to this library like that.



@dataclass
class AddHomeNoise:
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 very specific, is there a way how you can abstract it to noise from different datasets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@biphasic I have removed all noise related augmentations from this PR.


def __call__(self, audio):
SAMPLE_RIR = download_asset(
"tutorial-assets/Lab41-SRI-VOiCES-rm1-impulse-mc01-stu-clo-8000hz.wav"
Copy link
Member

Choose a reason for hiding this comment

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

this sort of hardcoded things I cannot merge into a public library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this sort of hardcoded things I cannot merge into a public library

I removed this hard coded audio path. Instead the room impulse audio needs to be passed by user. The corresponding test is also updated.

@biphasic
Copy link
Member

The Github Actions test suite doens't have a GPU installed, therefore there's no point in installing any CUDA dependencies. Tests should always run on CPU please. Thank you

@MinaKh
Copy link
Contributor Author

MinaKh commented Nov 27, 2023

The Github Actions test suite doens't have a GPU installed, therefore there's no point in installing any CUDA dependencies. Tests should always run on CPU please. Thank you

Thanks @biphasic, As far as I checked my tests are not running on GPU. The error might be caused by general imports of torch and torchaudio, or by the difference in my local version and the installed one on the server. So I included the versions in the requirements.
At this point I need to be able to run tests on GitHub to understand the issue better and fix it. Currently tests are not running automatically after my pushes (perhaps needs to be authorized every time by you and other admins?).

requirements.txt Outdated
Comment on lines 10 to 11
torch==1.12.0+cu113
torchaudio==0.12.0+cu113
Copy link
Member

Choose a reason for hiding this comment

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

cu113 means CUDA 11.3, so this is installing a fixed version of pytorch with CUDA backend

@biphasic
Copy link
Member

@MinaKh are the tests passing on your local machine?

@biphasic
Copy link
Member

Also I just relaxed the Github actions approval to the minimum level possible, I hope it now works without my manual approval!

@fabrizio-ottati
Copy link
Collaborator

@MinaKh I need to sit and do it properly. I will update you this afternoon

@fabrizio-ottati
Copy link
Collaborator

image

I don't know why but it keeps installing the CUDA version even if I have specificied to use the CPU wheel of PyTorch.

@fabrizio-ottati
Copy link
Collaborator

Okay, it seems I convinced it @MinaKh :)

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2023

Codecov Report

Attention: Patch coverage is 97.19626% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 77.72%. Comparing base (e5bd291) to head (0af124a).
Report is 22 commits behind head on develop.

Files Patch % Lines
tonic/audio_transforms.py 92.00% 2 Missing ⚠️
tonic/audio_augmentations.py 98.78% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #273      +/-   ##
===========================================
+ Coverage    76.84%   77.72%   +0.88%     
===========================================
  Files           53       55       +2     
  Lines         3001     3174     +173     
===========================================
+ Hits          2306     2467     +161     
- Misses         695      707      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MinaKh
Copy link
Contributor Author

MinaKh commented Dec 5, 2023

Okay, it seems I convinced it @MinaKh :)

Okay, it seems I convinced it @MinaKh :)

Thanks @fabrizio-ottati

@fabrizio-ottati
Copy link
Collaborator

@biphasic all the tests are passing now. I have created a separate test/torch_requirements.txt so that I can pull the CPU wheel of PyTorch. Moreover, following torchaudio documentation, specific combinations of torch and torchaudio versions need to be used to ensure safe installation.

Now the code is ready to be reviewed and CI should be safe.

@MinaKh
Copy link
Contributor Author

MinaKh commented Dec 5, 2023

Most transforms are encapsulated enough so that I can add them, but some stuff that uses QUTNoise things I won't be able to merge like that, unless it is made a bit more general. For example, maybe Tonic has a AddNoise class, but then in your user code at SynSense you call it with AddNoise(QUTNoise), after the principle of dependency injection

@biphasic I removed those classes (noise augmentations) from this branch. currently it is very specific and I will prepare another PR later for that.

@MinaKh MinaKh requested a review from biphasic December 5, 2023 13:18
@fabrizio-ottati
Copy link
Collaborator

What's the status on this PR? :)

@MinaKh
Copy link
Contributor Author

MinaKh commented Jan 22, 2024

What's the status on this PR? :)

It is ready for final review...

@biphasic biphasic marked this pull request as ready for review May 15, 2024 08:30
@biphasic biphasic merged commit 5a20a54 into neuromorphs:develop May 15, 2024
9 checks passed
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.

4 participants