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

Implement Ngram scorer #1946

Merged
merged 62 commits into from
May 25, 2020
Merged

Implement Ngram scorer #1946

merged 62 commits into from
May 25, 2020

Conversation

qmpzzpmq
Copy link
Contributor

@qmpzzpmq qmpzzpmq commented May 20, 2020

hi,

Here is Haoyu Tang from BIGO speech. I implement ngram with kenlm, and test it in aishell test since it works badly in BPE.

It could improve only e2e model, but still not as good as RNNLM. it might since I didn't tuning the decoding parameters.

@mergify
Copy link
Contributor

mergify bot commented May 20, 2020

This pull request is now in conflict :(

@mergify mergify bot added the conflicts label May 20, 2020
@mergify mergify bot removed the conflicts label May 20, 2020
@sw005320 sw005320 added ASR Automatic speech recogntion New Features labels May 20, 2020
@sw005320 sw005320 added this to the v.0.8.0 milestone May 20, 2020
@@ -36,3 +36,21 @@ exp/train_sp_pytorch_no_patience/decode_test_beam20_emodel.acc.best_p0.0_len0.0-
| SPKR | # Snt # Wrd | Corr Sub Del Ins Err S.Err |
| Sum/Avg | 7176 104765 | 92.2 7.6 0.2 0.2 8.0 50.2 |
```

# Ngram related
- there is no RNN not ngram
Copy link
Member

Choose a reason for hiding this comment

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

This result looks promising. Have you ever tried ngram-RNNLM interpolation (joint decoding)?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's my recommendation, too, and @qmpzzpmq is trying now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but there is no result better than currently one.
I guess it takes time to tuning the decoding weight parameters.

@qmpzzpmq
Copy link
Contributor Author

@ShigekiKarita
Hi, I am not I toally understand your means about test_ngram.py, but I still tried my best to finised to do it. This program to check is the score same with I got

test/test_ngram.py Outdated Show resolved Hide resolved
tools/Makefile Outdated Show resolved Hide resolved
test/test_ngram.py Outdated Show resolved Hide resolved
@qmpzzpmq
Copy link
Contributor Author

@ShigekiKarita is everything alright?

tools/Makefile Outdated Show resolved Hide resolved
Co-authored-by: b-flo <41155456+b-flo@users.noreply.github.com>
@kamo-naoyuki kamo-naoyuki self-requested a review May 25, 2020 13:04
@@ -0,0 +1,6 @@
ngram-weight: 1.0
beam-size: 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't you modify run.sh for tedliums2?
Is this a garbage file?

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

@@ -2,10 +2,11 @@ MAIN_ROOT=$PWD/../../..
KALDI_ROOT=$MAIN_ROOT/tools/kaldi

Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

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

@kamo-naoyuki
Copy link
Collaborator

@ShigekiKarita What do you think about the path of ngram scorer? I think espnet/nets/scorers is better.

Copy link
Contributor

@sw005320 sw005320 left a comment

Choose a reason for hiding this comment

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

LGTM.

egs/aishell/asr1/run.sh Outdated Show resolved Hide resolved
@@ -178,6 +182,26 @@ if [ ${stage} -le 3 ] && [ ${stop_stage} -ge 3 ]; then
--dict ${dict}
fi

ngramexpname=train_ngram
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you include this ngram LM training in stage 3?
I want to keep the role of the stage.

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

@sw005320
Copy link
Contributor

@ShigekiKarita What do you think about the path of ngram scorer? I think espnet/nets/scorers is better.

I agree.

@@ -176,8 +187,20 @@ if [ ${stage} -le 3 ] && [ ${stop_stage} -ge 3 ]; then
--valid-label ${lmdatadir}/valid.txt \
--resume ${lm_resume} \
--dict ${dict}

echo "stage 4: Ngram Preparation"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "stage 4: Ngram Preparation"
echo "stage 3: Ngram Preparation"

@sw005320
Copy link
Contributor

Very cool!
One more request.
Since this is a very cool feature, please add this n-gram extension in main REAME.md (https://github.com/espnet/espnet#key-features)

@mergify mergify bot added the README label May 25, 2020
@qmpzzpmq
Copy link
Contributor Author

qmpzzpmq commented May 25, 2020

Very cool!
One more request.
Since this is a very cool feature, please add this n-gram extension in main REAME.md (https://github.com/espnet/espnet#key-features)

@sw005320 done

@sw005320
Copy link
Contributor

I think this PR is almost ready. I just want to make sure that @ShigekiKarita would agree with putting this in espnet/nets/scorers.

@b-flo
Copy link
Member

b-flo commented May 25, 2020

Great work!
On another note, for your next PR you should try to group your commits a bit more (e.g.: consecutive commits with same target/purpose but small changes in each). I wanted to follow the changes but I had to unsubscribe to the PR because an email is sent for each commit you did (50+ in a few days!)...

@qmpzzpmq
Copy link
Contributor Author

qmpzzpmq commented May 25, 2020

@b-flo
yes, I will. since it is my first time to add a total new feature to espnet. I am not family with your test process, but now I know something about it.

Copy link
Member

@ShigekiKarita ShigekiKarita left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot for resolving our many requests!

@sw005320 sw005320 merged commit 263e3c6 into espnet:develop May 25, 2020
This was referenced May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASR Automatic speech recogntion CI Travis, Circle CI, etc New Features README
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants