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 recipe for Fluent Speech Commands dataset #1469

Merged
merged 9 commits into from
Jan 31, 2024

Conversation

HSTEHSTEHSTE
Copy link
Contributor

@pkufool
Copy link
Collaborator

pkufool commented Jan 22, 2024

Thanks! Could you please clean up the code first (for example, if your recipe share same source file, shall we symbolic to the existing one). Also please have a look at other recipes and follow the same structure.

@HSTEHSTEHSTE
Copy link
Contributor Author

Thanks! Could you please clean up the code first (for example, if your recipe share same source file, shall we symbolic to the existing one).

Thanks for your feedback!
I removed the tdnn-based recipe (as its performances were not competitive anyway). The transducer recipe was largely based on yesno, if you think it would be helpful I could symlink everything back to the yesno recipe (although I would be somewhat wary of doing so given the possibility of me making subsequent changes to this recipe). I have also not touched the copyrigh and licensing information on any of the files. Outside of these, if there's anything in particular where the code could be further cleaned up, please let me know. Thanks!

Also please have a look at other recipes and follow the same structure.
I have moved everything to fluent_speech_commands/SLU, following the dataset/task file structure. Is there anything else I need to fix?

@JinZr
Copy link
Collaborator

JinZr commented Jan 24, 2024

hi xinyuan! i left a few comments in this PR, please check.

also it would be more preferable to use symlink to reduce redundancy. if you want to make further changes to the recipe and make a PR in the future, you can try creating a new recipe like transducer_xx without using symlink in it.

thank you!

@JinZr
Copy link
Collaborator

JinZr commented Jan 24, 2024

usually we will use symlink for generic files like beam_search.py, conformer.py, decoder.py, encoder.py, encoder_interface.py, joiner.py, subsampling.py, model.py, transformer.py and test related scripts like test_*. and also rename the asr_datamodule.py to slu_datamodule.py considering the task name is "SLU" rather than "ASR" in your case 🤔

Xinyuan Li added 3 commits January 23, 2024 21:39
@HSTEHSTEHSTE
Copy link
Contributor Author

usually we will use symlink for generic files like beam_search.py, conformer.py, decoder.py, encoder.py, encoder_interface.py, joiner.py, subsampling.py, model.py, transformer.py and test related scripts like test_*. and also rename the asr_datamodule.py to slu_datamodule.py considering the task name is "SLU" rather than "ASR" in your case 🤔

Thanks a lot for your comments! In a recent commit I updated my recipe to use symlinks whenever possible (I think the only issue was beam_search which has a dependency on the vocabulary). I can't seem to see your comments on individual files... And they don't seem to be hidden behind a particular commit. Do you know how I could find them? Thanks again!

@JinZr
Copy link
Collaborator

JinZr commented Jan 24, 2024 via email

@JinZr
Copy link
Collaborator

JinZr commented Jan 24, 2024 via email

Signed-off-by: Xinyuan Li <[email protected]>
@HSTEHSTEHSTE
Copy link
Contributor Author

usually we will use symlink for generic files like beam_search.py, conformer.py, decoder.py, encoder.py, encoder_interface.py, joiner.py, subsampling.py, model.py, transformer.py and test related scripts like test_*. and also rename the asr_datamodule.py to slu_datamodule.py considering the task name is "SLU" rather than "ASR" in your case 🤔

Thanks a lot for your comments! In a recent commit I updated my recipe to use symlinks whenever possible (I think the only issue was beam_search which has a dependency on the vocabulary). I can't seem to see your comments on individual files... And they don't seem to be hidden behind a particular commit. Do you know how I could find them? Thanks again!

dear xinyuan, you can find the comments i left regarding specific files and lines in the "files changed" section as well as the "conversation" section of the webpage of this PR. it goes like this in the "conversation" section image best

On Wed, Jan 24, 2024 at 12:25 PM Henry Li Xinyuan @.> wrote: usually we will use symlink for generic files like beam_search.py, conformer.py, decoder.py, encoder.py, encoder_interface.py, joiner.py, subsampling.py, model.py, transformer.py and test related scripts like test_. and also rename the asr_datamodule.py to slu_datamodule.py considering the task name is "SLU" rather than "ASR" in your case 🤔 Thanks a lot for your comments! In a recent commit I updated my recipe to use symlinks whenever possible (I think the only issue was beam_search which has a dependency on the vocabulary). I can't seem to see your comments on individual files... And they don't seem to be hidden behind a particular commit. Do you know how I could find them? Thanks again! — Reply to this email directly, view it on GitHub <#1469 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOON42EUEXZWQRLYEI5OJPTYQCEMFAVCNFSM6AAAAABCCL7IB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBXGM2TKNZSGY . You are receiving this because you commented.Message ID: @.*>

I still couldn't see your comments...

github

I was quite puzzled by this until I stumbled upon this thread which seems to be what's happening:

https://github.com/orgs/community/discussions/30638#discussioncomment-4574199

So it seems you might need to "finish the review", whatever that means. Sorry for the hassle, and thanks again for helping with this PR!

@@ -169,7 +169,7 @@ def add_raw_counts_from_file(self, filename):
with open(filename, encoding=default_encoding) as fp:
for line in fp:
line = line.strip(strip_chars)
self.add_raw_counts_from_line(line)
self.add_raw_counts_from_line(line.split()[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

this line seems to be hard coded 🤔 not sure if it causes unwanted changes for other cases

@JinZr
Copy link
Collaborator

JinZr commented Jan 24, 2024

oof my bad, just submitted the review.

let me make sure of the ngram lm stuff before merging it.

thanks!

@HSTEHSTEHSTE
Copy link
Contributor Author

oof my bad, just submitted the review.

let me make sure of the ngram lm stuff before merging it.

thanks!

Thanks! It's been a while since I made that particular change (with ngram lms), I remember there was a good reason but I can't find it off the top of my head now, let me check again as well!

@HSTEHSTEHSTE
Copy link
Contributor Author

oof my bad, just submitted the review.
let me make sure of the ngram lm stuff before merging it.
thanks!

Thanks! It's been a while since I made that particular change (with ngram lms), I remember there was a good reason but I can't find it off the top of my head now, let me check again as well!

I tried running without the changes active: it appears that in the generated .arpa LM file, all the word indices were given a weight as well, so some weird interaction must have taken place between the 1-gram LM training and the add_raw_counts_from_line function. In theory I could add a check which runs the old version if n in n-gram is greater than or equal to 1, and run the new version if n=1, although I won't be able to justify my change with anything more convincing than "because it seems to be the only way that works and that doesn't break any existing recipes". What are your thoughts on this?

@JinZr
Copy link
Collaborator

JinZr commented Jan 25, 2024

yes i think the latest commit is ok for the special case.

waiting for the final CI test to be done, thank you!

@HSTEHSTEHSTE
Copy link
Contributor Author

Is it good to go? :)

@JinZr
Copy link
Collaborator

JinZr commented Jan 26, 2024

yes, i think this one is good to be merged once lhotse has merged the pr for data preparation

@HSTEHSTEHSTE
Copy link
Contributor Author

yes, i think this one is good to be merged once lhotse has merged the pr for data preparation

Thanks!! Looks like the lhotse PR has just been merged :)

@csukuangfj
Copy link
Collaborator

yes, i think this one is good to be merged once lhotse has merged the pr for data preparation

Thanks!! Looks like the lhotse PR has just been merged :)

Thanks for your contribution!

Let us merge it first so that further work won't be blocked.

Could you update the results and upload ptretrained models in a separate PR?

@HSTEHSTEHSTE
Copy link
Contributor Author

yes, i think this one is good to be merged once lhotse has merged the pr for data preparation

Thanks!! Looks like the lhotse PR has just been merged :)

Thanks for your contribution!

Let us merge it first so that further work won't be blocked.

Thanks!!

Could you update the results and upload ptretrained models in a separate PR?

Will do!

@csukuangfj csukuangfj merged commit b07d547 into k2-fsa:master Jan 31, 2024
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants