-
Notifications
You must be signed in to change notification settings - Fork 423
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
Librimix recipes #418
Librimix recipes #418
Conversation
😍 |
Oh yes, lots of review to do 😂 |
Concering the DCU performance, could it be because of different context (only 2 s)? Can you show training loss graphs of each of the models? Btw I'm surprised by ConvTasNet and DPRNN performance, it was much much worse for me (but I did dereverberation, not denoising) |
It might be because of the shorter context I can increase it if I reduce the batch size. I will send you the graphs.
Can you check one |
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, it looks good, thanks a lot.
Few comments to address that I highlighted in the code.
The duplicate on local
is a disaster, but that's bad design of Asteroid recipes indeed.
asteroid/dsp/declipp.py
Outdated
for i in range(len(est_np)): | ||
est_np[i] *= np.max(np.abs(mix_np)) / np.max(np.abs(est_np[i])) | ||
return est_np |
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.
- This modifies the array in-place, which is a bad idea.
- I'd use list comp here
mix_max = np.max(np.abs(mix_np)
return np.stack([est * mix_max / np.max(np.abs(est)) for est in est_np], dim=0)
- This needs a test.
egs/librimix/ConvTasNet/README.md
Outdated
| train-360 | sep_noisy | 12 | 12.5 | | ||
|
||
See available models [here](https://huggingface.co/JorisCos). |
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.
There are filters on the Hub:
- You can see all Asteroid models here: https://huggingface.co/models?filter=asteroid
- All Asteroid on Libri1Mix here: https://huggingface.co/models?filter=asteroid,dataset:Libri1Mix
So two things:
- I think we should have a LibriMix tag instead of Libri1Mix so that all LibriMix models are accessible from one query.
- For now, let's link to here: https://huggingface.co/models?filter=asteroid
asteroid/dsp/declipp.py
Outdated
import numpy as np | ||
|
||
|
||
def declipp(mix_np, est_np): |
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.
I'm unsure about the name. Declipping is the task of restoring the signal after clipping.
Maybe normalize_estimates(estimates, mixture)
would make sense? The name is less pretty but more explicit.
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.
I will change the name but should I keep est_np
as variable name to indicate that we only support np.array
for now ?
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.
Yes, keep the est_np
but if you can change the order of the variables it would be cool.
asteroid/dsp/declipp.py
Outdated
""" | ||
|
||
Args: | ||
mix_np (numpy array): One mixture |
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.
For typing to work, you need to use np.array
.
asteroid/dsp/declipp.py
Outdated
|
||
|
||
def declipp(mix_np, est_np): | ||
""" |
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.
Need a docstring explaining simply what the function does.
egs/librimix/DCCRNet/README.md
Outdated
@@ -0,0 +1,20 @@ | |||
### Results | |||
|
|||
The model was train for the `enh_single` task on Libri1Mix `train_360`. |
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.
The model was train for the `enh_single` task on Libri1Mix `train_360`. | |
The model was trained on the `enh_single` task on Libri1Mix `train_360`, at 16kHz. |
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 for all I guess.
egs/librimix/DCCRNet/local/conf.yml
Outdated
optim: | ||
optimizer: adam | ||
lr: 0.001 | ||
weight_decay: !!float 1e-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.
Missing new line
add normalization.py add new lines conf Fix readme black train
Fix linter please |
... When there is a commit on the browser, the tests are not ran 😑 There was again a linter problem, did you run black? |
I spoke to fast, the tests started ^^ |
I forgot the test for normalization I will request your review when the recipe is completed |
add test normalization_test.py
…recipes # Conflicts: # asteroid/dsp/normalization.py
Thanks again ! |
About this PR
This PR adds new recipes to the librimix dataset :
All the models are trained for speech enhancement on librimix 16khz train-360.
The pretrained models have been uploaded to Hugging Face (@jonashaag can you take a look at DCUNet the performance are quite poor does it looks similar to your experiments ? )
Beside the new recipes I changed the data generation of this recipe. The local data generation i.e the csv files with the path to the audio is separated from the LIbriMix generation (it should have been in the first place).
I also introduce a new function
declipp
for declipping audio before feeding it to the ASR models.There are a lot of duplicates in the code as I can't make the folder local a symbolic link. This probably falls under the discussion of the recipes' design.
I realize that it's a lot of files to check... all recipes have been tested and should work though.
Coming soon
Soon i will upload the pretrained models and results for SuDORMRFNet, SuDORMRFImprovedNet
I am also thinking about extending the evaluation with WER computation.