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

Added Delay Augmentation, tested on per_batch and per_example #171

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Pliploop
Copy link

Hi there,

I used this repo in a project of mine and ended up implementing my own custom delay augmentation using your boilerplate. I figured it would be useful for others so I'm contributing it here. Let me know if you want any changes made - I cooked it up rather fast but did test it pretty extensively on per-batch and per-example use cases. I don't think per-channel is a relevant use-case for delay, but could implement it if needed.

@iver56
Copy link
Collaborator

iver56 commented Jan 2, 2024

Thanks for the PR! I'll have a look later. In the meantime, could you please add a unit test or two?

@Pliploop
Copy link
Author

Pliploop commented Mar 8, 2024

Hi again @iver56 , sorry for the Delay ( :D ) In answering:

Tests have been added, and I changed to uniform sampling for some parameters.

Let me know if anything else is needed.

@iver56
Copy link
Collaborator

iver56 commented Mar 8, 2024

Thanks, I will have another look at it next week, probably on monday 👍

Copy link
Collaborator

@iver56 iver56 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 your efforts thus far. I have reviewed the PR and noted some change requests:

  • Set requires_sample_rate = True instead of False
  • Remove explicit "if not sample_rate" check in __init__, and use the sample_rate check in BaseWaveformTransform, which kicks in when you set requires_sample_rate = True
  • Allow passing sample_rate in either __init__ or in forward (i.e. when calling the transform), like what is possible in some other transforms in torch-audiomentations. I think this means you have to move some light calculations that are based on sample_rate from __init__ to randomize_parameters. Check other classes in torch-audiomentations for inspiration.
  • Resolve the git conflict
  • In order to align with the API style of the rest of the project, please change the style in which value ranges are passed. E.g. input min_volume_factor and max_volume_factor instead of volume_factor and volume_factor_range
  • Adjust the default parameters. Have a slightly wider range of repeats, attenuation, volume factor and delay. Also, I generally find the effect to be too subtle by default, so it would be cool if you could tune the defaults to allow for a more "extreme" result too.
  • format code with black

When it comes to the randomization of the delay time, I'm not sure if picking it linearly is best. Maybe exponential makes sense too in some cases? We could definitely use linear, but the possibility of exponential was a thought that struck me as I was going through the code. I'm open to input here. If you think linear is apt, let's roll with it for now.

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.

2 participants