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

2021 Clarity Challenge recipe #4210

Merged
merged 25 commits into from
Apr 14, 2022
Merged

2021 Clarity Challenge recipe #4210

merged 25 commits into from
Apr 14, 2022

Conversation

popcornell
Copy link
Contributor

This is an attempt to do a simple recipe for 2021 Clarity Challenge.

Currently missing: test set (I need to check if the oracle sources have been released for test set)
Also where do you set in recipes the path to the current root of the dataset ? What's the best practice ?

@mergify
Copy link
Contributor

mergify bot commented Mar 26, 2022

This pull request is now in conflict :(

@mergify mergify bot added conflicts ESPnet1 ESPnet2 ASR Automatic speech recogntion README labels Mar 26, 2022
� Conflicts:
�	espnet2/enh/layers/complex_utils.py
�	espnet2/enh/layers/dnn_beamformer.py
@mergify mergify bot removed the conflicts label Mar 26, 2022
@codecov
Copy link

codecov bot commented Mar 27, 2022

Codecov Report

Merging #4210 (0c470f9) into master (952a70a) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master    #4210   +/-   ##
=======================================
  Coverage   80.71%   80.71%           
=======================================
  Files         453      453           
  Lines       39578    39578           
=======================================
+ Hits        31945    31946    +1     
+ Misses       7633     7632    -1     
Flag Coverage Δ
test_integration_espnet1 67.13% <ø> (ø)
test_integration_espnet2 50.06% <50.00%> (+<0.01%) ⬆️
test_python 67.16% <0.00%> (ø)
test_utils 24.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
espnet2/bin/enh_scoring.py 94.20% <50.00%> (+1.44%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@sw005320 sw005320 added Recipe SE Speech enhancement and removed ASR Automatic speech recogntion ESPnet1 labels Mar 27, 2022
@sw005320 sw005320 added this to the v.0.10.7 milestone Mar 27, 2022
egs2/clarity/enh_2021/run.sh Outdated Show resolved Hide resolved
egs2/clarity/enh_2021/run.sh Outdated Show resolved Hide resolved
egs2/clarity/enh_2021/run.sh Outdated Show resolved Hide resolved
@popcornell
Copy link
Contributor Author

Tests fails but probably not related to this PR


. utils/parse_options.sh


Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a check for the existence of sox (as it is assumed to be available in local/prep_data.py) like the following?

! command -v sox &>/dev/null && echo "sox: command not found" && exit 1;

"(Folder containing train, dev and metadata dirs)",
)
parser.add_argument(
"--fs",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this argument used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is used now

Comment on lines 1 to 4
import argparse
import os
import json
from pathlib import Path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you fix the import order to follow the alphabet order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Emrys365
Copy link
Collaborator

Emrys365 commented Apr 8, 2022

BTW, You may need to merge the latest master branch to fix the CI test error.

@popcornell
Copy link
Contributor Author

Thanks for your suggestions ! Now it should be better.

@kan-bayashi kan-bayashi modified the milestones: v.0.10.7, v.202205 Apr 11, 2022
Copy link
Collaborator

@Emrys365 Emrys365 left a comment

Choose a reason for hiding this comment

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

LGTM now. Just two last comments:

  • Could you add your recipe to egs2/README.md?
  • Could you rename the recipe to clarity21 to distinguish from the challenges in other years?

@sw005320
Copy link
Contributor

Can you upload a model to the HF hub and add a link to README.md?

@popcornell
Copy link
Contributor Author

Sure, I'll look into how to do it

@popcornell
Copy link
Contributor Author

Could you rename the recipe to clarity21 to distinguish from the challenges in other years?

Isn't it better to use a subfolder as I am using right now ? I am using enh_2021 subfolder

@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2022

This pull request is now in conflict :(

@mergify mergify bot added the conflicts label Apr 12, 2022
@Emrys365
Copy link
Collaborator

Could you rename the recipe to clarity21 to distinguish from the challenges in other years?

Isn't it better to use a subfolder as I am using right now ? I am using enh_2021 subfolder

It seems that espnet prefers to split them into different recipes, like chime4 and chime5. So it's better to also follow the convention here.

@mergify mergify bot removed the conflicts label Apr 13, 2022
@popcornell
Copy link
Contributor Author

https://huggingface.co/popcornell/clarity21_train_enh_beamformer_mvdr
Added the pre-trained model on HF

@sw005320 sw005320 merged commit 0efccca into espnet:master Apr 14, 2022
@sw005320
Copy link
Contributor

Thanks, @popcornell!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants