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

AugDiskCachedDataset to map the copy index to augmentation parameter #274

Merged
merged 17 commits into from
May 30, 2024

Conversation

MinaKh
Copy link
Contributor

@MinaKh MinaKh commented Nov 24, 2023

This branch added a child class for DiskCachedDataset called AugDiskCachedDataset.
Its main use is for a family of so-called deterministic augmentations with a rather discrete parameter space. For instance a noise augmentation on audio samples in which SNR can have only 5 values.

  • In DiskCachedDataset num_copies can be used to generate N copies of a data sample. This is ok when used transforms/augmentations have an infinite/probabilistic parameter space. So the chance of generating repetitive augmented versions is very low.
  • On the other hand for deterministic augmentations with N parameter it is advantageous to map the copy index to the parameter to avoid re-generating existing samples and to make sure generated copies cover all desired parameter space .
  • The main feature of this class is that the index of file copy is mapped to the parameter of augmentation

@MinaKh MinaKh marked this pull request as draft November 24, 2023 15:02
@biphasic
Copy link
Member

Hello @MinaKh! Currently I don't understand how what you're trying to achieve with this class cannot already be done with existing classes.
It seems to me that you want to control the augmentations exactly, but then I don't understand why they're called augmentations. Can you please

  • provide an example of how you use your proposed class
  • explain with a concrete example why the current code cannot do what you need to do

Before I can merge this, this class would need a test as well, it might be helpful to add that as well.

@codecov-commenter
Copy link

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (db13037) 76.80% compared to head (c9d26b0) 77.34%.
Report is 12 commits behind head on develop.

Files Patch % Lines
tonic/cached_dataset.py 58.69% 19 Missing ⚠️
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     #274      +/-   ##
===========================================
+ Coverage    76.80%   77.34%   +0.53%     
===========================================
  Files           53       54       +1     
  Lines         3001     3165     +164     
===========================================
+ Hits          2305     2448     +143     
- Misses         696      717      +21     

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

@MinaKh
Copy link
Contributor Author

MinaKh commented Dec 20, 2023

Hello @MinaKh! Currently I don't understand how what you're trying to achieve with this class cannot already be done with existing classes. It seems to me that you want to control the augmentations exactly, but then I don't understand why they're called augmentations. Can you please

  • provide an example of how you use your proposed class
  • explain with a concrete example why the current code cannot do what you need to do

Before I can merge this, this class would need a test as well, it might be helpful to add that as well.

Hi @biphasic! Thanks for your feedback.

  • Deterministic augmentations are not uncommon, specially in audio processing and they are still called augmentation but in a more controlled way.
  • I have added a notebook to docs/tutorails/Aug_DiskCachDataset.ipynb and have addressed your raised point there with a synthetic dataset. Please let me know if it is not clear.
  • I also have added a test test/test_aug_caching.py which is pretty similar to what I presented in the notebook. Please let me know if you have other ideas for tests.
  • this branch has been merged with the branch of my other PR : adding audio transforms.....

@biphasic biphasic marked this pull request as ready for review May 15, 2024 14:13
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.

Hello Mina, I have two small changes that I request, then I can merge this

from warnings import warn

import h5py
import numpy as np
from torchvision.transforms import Compose
Copy link
Member

Choose a reason for hiding this comment

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

Tonic should work without having torch installed, can you move this line to wherever it is used? So just wherever torchvision.transforms.Compose is used, import it one line above.
During testing and documentation, of course we can say that torch must be installed. That's why torch requirements are only used in the testing and documentation Github Actions steps

test/requirements.txt Outdated Show resolved Hide resolved
test/test_aug_caching.py Outdated Show resolved Hide resolved
test/test_aug_caching.py Outdated Show resolved Hide resolved
@biphasic biphasic merged commit d67aa25 into neuromorphs:develop May 30, 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.

3 participants