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.

Sign up for GitHub

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

Jump to bottom

tedx_spanish_corpus egs2 recipe #4523

Merged
merged 11 commits into from
Aug 1, 2022
Merged

tedx_spanish_corpus egs2 recipe #4523

merged 11 commits into from
Aug 1, 2022

Conversation

jessicah25
Copy link
Contributor

PR for TEDx Spanish Corpus (https://www.openslr.org/67/)

@mergify
Copy link
Contributor

mergify bot commented Jul 21, 2022

This pull request is now in conflict :(

@mergify mergify bot added conflicts ESPnet1 ESPnet2 ASR Automatic speech recogntion MT Machine translation README Installation labels Jul 21, 2022
@sw005320 sw005320 requested a review from ftshijt July 21, 2022 15:40
ftshijt
ftshijt requested changes Jul 22, 2022
View reviewed changes
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.

Many thanks for your contribution. I guess you are using the wrong version of black to format the codebase, resulting in many changes to other parts. Could you try to revert the changes to those, otherwise you may not pass CI tests for the formating. Also, I have some minor suggestions as follows:

egs2/tedx_spanish_corpus_openslr67/asr1/RESULTS.md Outdated Show resolved Hide resolved
egs2/tedx_spanish_corpus_openslr67/asr1/conf/train_asr.yaml Outdated Show resolved Hide resolved
egs2/tedx_spanish_corpus_openslr67/asr1/local/create_data.py Outdated Show resolved Hide resolved
egs2/tedx_spanish_corpus_openslr67/asr1/local/train.txt Outdated Show resolved Hide resolved
@mergify mergify bot removed the conflicts label Jul 26, 2022
sw005320
sw005320 reviewed Jul 26, 2022
View reviewed changes
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.

I have some comments.

egs2/README.md Outdated Show resolved Hide resolved
egs2/TEMPLATE/asr1/db.sh Outdated Show resolved Hide resolved
@sw005320 sw005320 added Recipe and removed Installation MT Machine translation README ESPnet1 labels Jul 28, 2022
@sw005320 sw005320 added this to the v.202207 milestone Jul 28, 2022
@sw005320
Copy link
Contributor

Why some files that are not related to this PR are changed?
black is wrongly applied?

@mergify
Copy link
Contributor

mergify bot commented Jul 29, 2022

This pull request is now in conflict :(

@mergify mergify bot added the conflicts label Jul 29, 2022
ftshijt
ftshijt reviewed Jul 29, 2022
View reviewed changes
Copy link
Collaborator

@ftshijt ftshijt left a comment •
edited

Choose a reason for hiding this comment

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

Please fix the conflict~ A minor issue is as follows:

egs2/tedx_spanish_openslr67/asr1/local/data.sh
Comment on lines +49 to +50
# create train, dev, test split 90/5/5
# python local/split_data.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

The line is commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I release the code for the way the data was split? this is where it sits; but we don't run it after the initial split which is now uploaded in a separate repo

@jessicah25
Copy link
Contributor Author

jessicah25 commented Aug 1, 2022 •
edited

Why some files that are not related to this PR are changed? black is wrongly applied?

I am currently running black on 22.6.0, is this the correct version?

@sw005320
Copy link
Contributor

sw005320 commented Aug 1, 2022

I found that

  • egs2/TEMPLATE/asr1/pyscripts/utils/plot_sinc_filters.py
  • egs2/dirha_wsj/asr1/local/prepare_dirha_wsj.py
  • egs2/misp2021/avsr1/local/prepare_far_video_roi.py
  • espnet/nets/pytorch_backend/transducer/rnn_decoder.py
  • espnet2/asr/transducer/transducer_decoder.py
    were changed.
    They are not related to this PR and it should not be changed.

Also, please fix the conflict.

@mergify mergify bot removed the conflicts label Aug 1, 2022
@sw005320 sw005320 removed the ESPnet1 label Aug 1, 2022
@mergify mergify bot added the ESPnet1 label Aug 1, 2022
@codecov
Copy link

codecov bot commented Aug 1, 2022 •
edited

Codecov Report

Merging #4523 (991ec25) into master (b5a8f5b) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4523   +/-   ##
=======================================
  Coverage   82.52%   82.52%           
=======================================
  Files         487      487           
  Lines       42024    42024           
=======================================
+ Hits        34680    34681    +1     
+ Misses       7344     7343    -1
Flag Coverage Δ
test_integration_espnet1 66.36% <ø> (ø)
test_integration_espnet2 48.24% <ø> (ø)
test_python 69.76% <ø> (+<0.01%) ⬆️
test_utils 23.29% <ø> (ø)

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

Impacted Files Coverage Δ
espnet/distributed/pytorch_backend/launch.py 83.90% <0.00%> (+1.14%) ⬆️

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

@sw005320 sw005320 added the auto-merge Enable auto-merge label Aug 1, 2022
@mergify mergify bot merged commit 5115eb9 into espnet:master Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sw005320 sw005320 sw005320 left review comments

@ftshijt ftshijt ftshijt requested changes

Assignees
No one assigned
Labels
ASR Automatic speech recogntion auto-merge Enable auto-merge ESPnet1 ESPnet2 README Recipe
Projects
None yet
Milestone
v.202207
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jessicah25 @sw005320 @ftshijt