-
Notifications
You must be signed in to change notification settings - Fork 0
Newnnet3 #1
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
base: master
Are you sure you want to change the base?
Newnnet3 #1
Conversation
|
OK, here are my initial thoughts. |
|
Hi @danpovey where are your comments? Or was it just the above one? |
danpovey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, missed the actual review
egs/mini_librispeech/s5b/cmd.sh
Outdated
| # or search for the string 'default_config' in utils/queue.pl or utils/slurm.pl. | ||
|
|
||
| export train_cmd="queue.pl -l q1d -l io_big -V" | ||
| export decode_cmd="queue.pl -l q1d -l io_big -V" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if you don't put things here that are specific to your grid.
Also I'm not convinced that you need to copy the whole mini_librispeech recipe if you are just changing the chain stuff. We'll have this issue elsewhere too. Perhaps we could use the subdirectory name chain2 for the new script style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now deleted the s5b recipe. I have create steps/nnet3/chain2 and a corresponding soft link steps/chain2. I have also created local/chain2 and moved the new code to all these folders.
| # matched topology (since it gets the topology file from the model). | ||
| utils/mkgraph.sh \ | ||
| --self-loop-scale 1.0 data/lang_test_tgsmall \ | ||
| $tree_dir $tree_dir/graph_tgsmall || exit 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file looks unchanged to me, vs. the old one... makes it hard to see what the changes are if lots of files are duplicated. (And we wouldn't want to duplicate unnecessary files in the real thing.) But I'll try to figure out where the important stuff is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the updated PR, you will no longer have 100+ files to review. Sorry about this.
| #!/bin/bash | ||
|
|
||
| # Copyright 2019 Johns Hopkins University (Author: Daniel Povey). Apache 2.0. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re the naming of this: perhaps it would be better to have steps/nnet3/chain2/train.sh, and put the other things related to this in the chain2 directory. easier for discoverability and gives more structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, I created steps/nnet3/chain2 in egs/wsj/s5. In both minilibrespeech and swb, there are now local/chain2 folders.
| @@ -0,0 +1,385 @@ | |||
| #!/bin/bash | |||
|
|
|||
| # run_tdnn_8k.sh is like run_tdnn_7k.sh but uses new kaldi recipe. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you have understood the naming scheme right... we normally go 1a, 1b etc., and if we use up the letters, go to 2a. We don't normally increase the number while leaving the letter the same.
Let's take "chaina" out of all of the names, but we can use chain. For example: exp/chain2. And steps/chain2 -> steps/nnet3/chain2, as a soft link, just like steps/chain -> steps/nnet3/chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem was that I was trying to re-use the local/chain/tuning folders. But now, I have changed the folder organisation, so the numbering will be consistent.
egs/swbd/s5d/RESULTS
Outdated
| @@ -0,0 +1,54 @@ | |||
| # Baseline results with tdnn_7k recipe | |||
| cat exp/chain/tdnn_7k_sp/decode_eval2000_sw1_fsh_fg/score_*/*swbd.filt.sys | fgrep Sum | sort -k11,11 -g | head | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not how we display the results... comparing across all acwt's is very unreadable. Instead you can use local/chain/compare_wer.sh, or use the commands in the old RESULTS file ,,, things like
grep Sum exp/chain/tdnn_7k_sp/decode_eval2000_sw1_fsh_fg/score_*/*swbd.filt.sys | utils/best_wer.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed now. But there will no longer be a new recipe for swbd. I just modified the RESULTS file in swbd/s5c
| @@ -0,0 +1,17 @@ | |||
| # you can change cmd.sh depending on what type of queue you are using. | |||
| # If you have no queueing system and want to run on a local machine, you | |||
| # can change all instances 'queue.pl' to run.pl (but be careful and run | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't duplicate the entire swbd recipe (c->d) unless there is something going on early in data preparation that would make things incompatible. We need to structure these new scripts so that they can slot into existing recipes that have already been run and that already have scripts, without invalidating the existing stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed swbd/s5d recipes. There no new recipes in the updated PR.
| #end configuration section. | ||
|
|
||
| echo "$0 $@" # Print the command line for logging | ||
| exit 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there was an error in this script LMK what it was!
I think we have fixed bugs in this script in the past... possibly your code base was not merged with the latest master changes when you ran this stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reverted this. This was a problem for me at Idiap, but I verified with other people here. They don't have this problem. So, I'll create an issue in the main Kaldi repo if I get this error again.
| echo "$0 $@" # Print the command line for logging | ||
|
|
||
| [ -f ./path.sh ] && . ./path.sh | ||
| #[ -f ./path.sh ] && . ./path.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to revert these changes.
LMK what the problem is.. we should fix it in the right way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a constant problem for me when running things on the grid here at Idiap. So, I commented it. Sourcing the path multiple times sends the jobs to Eqw. I have reverted this in the PR to master. Again, I will create an issue in Kaldi if I can reproduce the problem consistently.
| fi | ||
| echo "$0: Accumulating LDA stats" | ||
| $cmd JOB=1:$nj $ldafolder/log/acc.JOB.log \ | ||
| nnet3-chain-acc-lda-stats $lda_acc_opts --rand-prune=${rand_prune} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an FYI: I am trying to move away from this preconditioning stuff and instead use deltas, which are easier logistically. E.g. see egs/mini_librispeech/s5/local/chain/tuning/run_tdnn_1j.sh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you have mentioned this to me over e-mail. But I had to implement this to make sure there is no difference from the existing scripts, so that I can make a fair comparison. I can remove it if you want.
| set -euo pipefail | ||
|
|
||
| stage=0 | ||
| train_set=train_clean_5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like it belongs in steps! It looks like a local script! Also I would like to move away from speed perturbation and toward noise and reverb augmentation, so I'm not sure that I want to bake the speed perturbation into the script like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was a mistake. I didn't use this script anywhere (at least not the one in this location). I have removed it from the repo.
|
Hi Dan, Srikanth |
|
Thanks!
…On Mon, Oct 7, 2019 at 8:47 AM Srikanth M R ***@***.***> wrote:
Hi Dan,
Thanks for all the comments. It will take me a couple of days to look at
them and reply as I'm facing a couple of deadlines today and tomorrow.
Srikanth
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1?email_source=notifications&email_token=AAZFLO7DPENXKPSWWG34OALQNNKZJA5CNFSM4IW2TVEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAQ23TQ#issuecomment-539078094>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAZFLO7PGZXWFIIAMCT6VHTQNNKZJANCNFSM4IW2TVEA>
.
|
… egs/mini_librispeech
… build_tree.sh from run_tdnn_2a.sh
align_lats.sh get_raw_egs.sh process_egs.sh randomize_egs.sh train2.sh (didn't know what else to call it) validate_processed_egs.sh validate_randomized_egs.sh validate_raw_egs.sh choose_egs_to_merge.py get_train_schedule.py
…related variables
…ibrespeech recipe
local/chain/tuning/run_tdnn_2a.sh: works now, but the WER is 5% worse than expected local/chain/tuning/run_tdnn_1a_sans_ivectors.sh: version of local/chain/tuning/run_tdnn_1a.sh that doesn't use ivectors local/chain/data_prep_common.sh: useful for new type of chain scripts
…te LDA for chain models
…for chain model scripts
…tors. looks for transition model in two places.
…f commented code. but committing it because it works and the code is moving towards what we want
…_input_frames is now obtained from data folder
…ne2.cc the latter is useful for final model combination in the new chain training recipes the former is a header file I forgot to add under git
…eps/nnet3/chain2/
…5b/local/.., but there is no need to have a new recipe
- deleted s5b recipe in minlibrespeech
… is now the same as what is there in master
…h_dir from options
…/chain2/tuning/run_tdnn_7k.sh
…s to be reviewed for PR). these files originally had changes adapted to the Idiap environment: egs/wsj/s5/steps/nnet3/chain/get_egs.sh egs/wsj/s5/steps/nnet3/chain/train.py egs/wsj/s5/steps/online/nnet2/train_ivector_extractor.sh egs/swbd/s5c/local/chain/tuning/run_tdnn_7k.sh
egs/wsj/s5/steps/online/nnet2/train_diag_ubm.sh egs/wsj/s5/steps/online/nnet2/train_ivector_extractor.sh this simplifies PR a bit
… from the commits, it was there in the chaina branch, but it is no longer useful
No description provided.