-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Update-LID using DNN #999
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
Update-LID using DNN #999
Conversation
|
@s-mousmita, thanks for the update. Could you do the following:
|
|
The point is that we don't want to be maintaining two copies of lid/, one in egs/lre07/v1 and one in egs/lre07/v2 . See what we did in egs/sre10/v1/sid and egs/sre10/v2/sid. They both point to egs/sre08/v1/sid . |
|
Yes, sorry but script name? |
egs/lre07/README.txt
Outdated
| Language Evaluation. The subdirectory v1 demonstrates the standard LID | ||
| system, which is an I-Vector based recipe using full covarience GMM-UBM and logistic regression model. | ||
| The subdirectory v2 demonstrates the LID sysem using a Time Delay Deep Neural Network based UBM | ||
| which is used to replace the GMM-UBM of v1. The DNN is trained using about 1800 hours of the english portion of Fisher. |
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 be mindful of typos here. E.g., covarience -> covariance, and sysem - > system.
Also, please format the text so that lines don't go over 80 columns.
|
@s-mousmita, please let me know when you've addressed the comments |
|
Thanks @david-ryan-snyder .Please check updated scripts. |
|
|
||
| nnet_feats="ark,s,cs:apply-cmvn-sliding --center=true scp:$sdata_dnn/JOB/feats.scp ark:- |" | ||
|
|
||
| ## Set up SDC features. |
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.
Could you remove the extra '#'?
|
I just had a look at it... Also be careful that sometimes the program pathname appears in usage messages (it's wrong in some cases currently)-- grepping for steps/nnet2 and local/dnn in those scripts should find all the places you'd need to change the path. |
|
I have removed local/dnn and created lid/nnet2. Paths local/dnn and steps/nnet2 at various places of the scripts have been changed to lid/nnet2. |
|
I don't think we should move everything from local/dnn to lid/nnet2. My suggestion:
|
|
@david-ryan-snyder The following are correct?
|
|
train_multisplice_accel2.sh is not dataset-specific so it should go into Dan On Sat, Sep 3, 2016 at 1:47 PM, Mousmita Sarma notifications@github.com
|
dae5139 to
d73b43d
Compare
|
I have made the changes suggested above. |
|
Would you mind trying to run it one more time before we commit it? On Sat, Sep 3, 2016 at 3:41 PM, Mousmita Sarma notifications@github.com
|
|
I agree with the script locations in the current commit. |
|
Dan, Nagendra On Sat, Sep 3, 2016, 3:42 PM Daniel Povey notifications@github.com wrote:
|
|
Yes, that sounds good. On Sun, Sep 4, 2016 at 12:24 AM, Nagendra Goel notifications@github.com
|
|
I assume someone is still working on this? |
|
Training is running from scratch on JHU machines. On Tue, Sep 20, 2016, 7:41 PM Daniel Povey notifications@github.com wrote:
|
|
@ngoel17, any progress on this? |
b482d15 to
76fb9da
Compare
|
I have updated the scripts. |
david-ryan-snyder
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.
I think most of it looks good. I think the main thing I'd like to see updated is that there's a huge file that is added to the commit that could instead be a symbolic link.
| --transform-dir "$transform_dir" --online-ivector-dir "$online_ivector_dir" \ | ||
| --iter $x $data $lang $dir $dir/ali_$time || exit 1 | ||
|
|
||
| lid/nnet2/relabel_egs2.sh --cmd "$cmd" --iter $x $dir/ali_$time \ |
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.
Are you sure we can't use steps/nnet2/relabel_egs2.sh here?
| $egs_in $egs_out || exit 1 | ||
| fi | ||
|
|
||
| echo "$0: Finished relabeling training examples" |
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.
Are you sure we need this script? Can we not use the one in steps/nnet2/relabel_egs2.sh?
| export decode_cmd=run.pl | ||
| export cuda_cmd=run.pl | ||
| export mkgraph_cmd=run.pl | ||
|
|
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 way the queue options are handled was changed in this commit: b3bbc03
Basically you need to update the comment and the export statements to be more similar to what you see in the link above. The default commands should include queue.pl in it.
| return -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.
For files like this, that are unlikely to ever be modified, it's better to make a symbolic link to the same file in egs/lre07/v1/local.
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 probably don't need to do that for any of the other scripts.
egs/lre07/README.txt
Outdated
| Language Evaluation. The subdirectory v1 demonstrates the standard | ||
| LID system, which is an I-Vector based recipe using full covariance | ||
| GMM-UBM and logistic regression model. The subdirectory v2 demonstrates | ||
| the LID system using a Time Delay Deep Neural Network based UBM |
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 would consider replacing "Time Delay Deep Neural Network" with "time-delay deep neural network."
|
@david-ryan-snyder Scripts are updated according to your recent comments. |
fc35eba to
764803b
Compare
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 think it's ready. @danpovey, do you have any lingering doubts before accepting it?
Actually N/M since the check failed. Let's wait until we figure that out.
|
@danpovey it looks like the error is in exp-test. Have you seen this in recent PRs? I'm thinking this is probably not related to @s-mousmita's updates.
|
|
That exp-test thing was not what was causing the build to fail, it's normal On Sun, Oct 30, 2016 at 11:41 AM, david-ryan-snyder <
|
|
Thanks! Merging. |
|
@danpovey @david-ryan-snyder Thank you for approving the recipe. |
@david-ryan-snyder @danpovey Adding update.
Changes are made with reference to the old pull request.