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

Fix aishell3 data preparation script #4277

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Conversation

LanceaKing
Copy link
Contributor

No description provided.

@mergify mergify bot added the ESPnet2 label Apr 14, 2022
@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #4277 (5a86035) into master (fba5041) will not change coverage.
The diff coverage is n/a.

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

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

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

@sw005320 sw005320 added this to the v.202205 milestone Apr 14, 2022
@sw005320 sw005320 requested a review from ftshijt April 14, 2022 11:42
@sw005320
Copy link
Contributor

@LanceaKing, thanks a lot.
Could you tell me how this PR fixes the issue with an example?

@ftshijt
Copy link
Collaborator

ftshijt commented Apr 17, 2022

I can somehow understand the intention of this fix. If we set as bool in argparse, it would be set as true as long as the argument is presented during the function call (even if it is false). The external_g2p is set as True as default, so the external_g2p will always be set as True.

Copy link
Collaborator

@ftshijt ftshijt left a comment

Choose a reason for hiding this comment

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

LGTM! Many thanks

@LanceaKing
Copy link
Contributor Author

LanceaKing commented Apr 17, 2022

I can somehow understand the intention of this fix. If we set as bool in argparse, it would be set as true as long as the argument is presented during the function call (even if it is false). The external_g2p is set as True as default, so the external_g2p will always be set as True.

As @ftshijt said, the argument external_g2p in the previous code is always True unless passing an empty string. Also, the previous code would add spaces between Chinese characters, which is not correct. @sw005320

@ftshijt ftshijt merged commit a457d34 into espnet:master Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants