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

Fixing some issues with chime7-task1 baseline #4925

Merged
merged 9 commits into from Feb 9, 2023

Conversation

popcornell
Copy link
Contributor

many thanks to Kamo Naoyuki @kamo-naoyuki .
I think there was a mistake on the close talk microphones for dipco dev (they were removed for eval).
I will also address your other useful comments but I am unsure about the sox one.

@popcornell popcornell mentioned this pull request Feb 8, 2023
@mergify mergify bot added the ESPnet2 label Feb 8, 2023
@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #4925 (ecf3bd3) into master (7cf99cf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4925   +/-   ##
=======================================
  Coverage   76.56%   76.56%           
=======================================
  Files         603      603           
  Lines       53753    53753           
=======================================
  Hits        41157    41157           
  Misses      12596    12596           
Flag Coverage Δ
test_integration_espnet1 66.33% <ø> (ø)
test_integration_espnet2 47.58% <ø> (ø)
test_python 66.44% <ø> (ø)
test_utils 23.35% <ø> (ø)

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

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kamo-naoyuki
Copy link
Collaborator

@popcornell About dipco, this fix works for me. Thank you!

@kamo-naoyuki
Copy link
Collaborator

I got the following error at stage4

Stage 4: Enhance segments using GSS
local/run_gss.sh: line 54: affix[@]: unbound variable

I guess it can be fixed by

"${affix[@]}" -> ${affix[@]:-}

However, I'm not sure why using an array variable i.e. affix=().

@popcornell
Copy link
Contributor Author

popcornell commented Feb 9, 2023

I am using that variable because I want that when channels is undefined it is not passed so GSS uses all available ones.

It runs fine on my side so this is strange. What does ${affix[@]:-} mean ? I am not a great bash expert

@kamo-naoyuki
Copy link
Collaborator

kamo-naoyuki commented Feb 9, 2023

I am using that variable because I want that when channels is undefined it is not passed so GSS uses all available ones.

The following is enough for your case. It is not necessary to use array variable.

  affix=
  if ! [ $channels == all  ]; then
    affix+="--channels=$channels"
  fi

It runs fine on my side so this is strange. What does ${affix[@]:-} mean ? I am not a great bash expert

This script obviously can't work with "set -u" if channels != all i.e. $channels is empty.

Please check.

  • ${foo:-} means returning a empty string if foo is undefined (This is a workaround in the case of using an empty array variable with set -u.)
  • If you give "${foo}" to gss command, it is interpreted as an argument even if foo is empty and it causes an arguments error for gss command. ${foo} can avoid this problem.

@popcornell
Copy link
Contributor Author

popcornell commented Feb 9, 2023

I am starting to add this pretrained model https://huggingface.co/popcornell/chime7_task1_asr1_baseline
WER (%) on (GSS on dev) of

  1. CHiME-6 35.5
  2. DiPCo 39.3
  3. Mixer 6 20.6

Note that now i discarded sentences < 0.2 seconds (not many on dev, so it won't change much) so it is not true WER for the challenge.

# ESPNet does not scale parameters with num of GPUs by default, doing it
# here for you
asr_batch_size=$(calc_int 128*$ngpu) # reduce 128 bsz if you get OOMs errors
asr_max_lr=$(calc_float $ngpu/10000.0)
asr_warmup=$(calc_int 40000.0/$ngpu)

if [ $decode_only == 1 ]; then
Copy link
Contributor Author

@popcornell popcornell Feb 9, 2023

Choose a reason for hiding this comment

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

if decode only i run data prep and GSS only on development sets

@YoshikiMas
Copy link
Contributor

I faced minor issues in install_dependencies.sh.
I'm not sure but I think this line could be ${MAIN_ROOT}/tools/installers/install_s3prl.sh.
Also, this line should be ${MAIN_ROOT}/tools/installers/install_gss.sh.

@popcornell
Copy link
Contributor Author

Indeed thanks Yoshiki !

@sw005320 sw005320 added Recipe ASR Automatic speech recogntion Bugfix labels Feb 9, 2023
@sw005320 sw005320 added this to the v.202303 milestone Feb 9, 2023
@sw005320 sw005320 added the auto-merge Enable auto-merge label Feb 9, 2023
@sw005320
Copy link
Contributor

sw005320 commented Feb 9, 2023

LGTM
Once the CI check is passed, I'll merge this.

@popcornell
Copy link
Contributor Author

Note that I will need to change the README.md too probably based on what we decide tomorrow about the channel selection stuff.

@popcornell
Copy link
Contributor Author

popcornell commented Feb 9, 2023

Rules are not clear not if

if [ ${dset_name} == dipco ]; then
should be allowed or not. IMO it should be allowed but then we have to distinguish it from automatic or manual domain classification.
The model remains the same (we want to discourage one model for each scenario) but you just select a bunch of microphones with some a priori knowledge for example of the topology of the array (e.g. linear in chime6 and i use only outer mics)

@mergify mergify bot merged commit ffbf7e0 into espnet:master Feb 9, 2023
@sw005320
Copy link
Contributor

sw005320 commented Feb 9, 2023

OK, please make additional PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASR Automatic speech recogntion auto-merge Enable auto-merge Bugfix ESPnet2 Recipe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants