-
Notifications
You must be signed in to change notification settings - Fork 221
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
Feature/volume perturbation #382
Feature/volume perturbation #382
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.
Wow great work and nice test coverage! I left a couple of comments. I’m also thinking that naming it perturb_volume rather than perturb_vol would be more consistent with other Lhotse names (we have usually avoided abbreviations in APIs so far).
|
||
def __init__( | ||
self, | ||
factors: Union[float, List[float]], |
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.
factors: Union[float, List[float]], | |
factors: Union[float, Sequence[float]], |
This allows tuples without type checker conflicts
|
||
|
||
def test_deserialize_transform(audio): | ||
speed = AudioTransform.from_dict({'name': 'Speed', 'kwargs': {'factor': 1.1}}) | ||
perturbed = speed(audio, SAMPLING_RATE) | ||
assert perturbed.shape == (1, 14545) |
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.
These changes would ideally be introduced in a copy of this unit test (my principle is one logical assertion == one unit test, to avoid tests that check two or more independent functionalities because they are confusing to readers and harder to debug)
test/augmentation/test_torchaudio.py
Outdated
speed = AudioTransform.from_dict(data) | ||
perturbed = speed(audio, SAMPLING_RATE) | ||
assert perturbed.shape == (1, 14545) | ||
vol_orig = Vol(factor=0.5) |
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.
Same comment as above
I split tests and replaced |
Oh yeah that's a good catch. Thanks a lot! |
Add volume perturbation