WIP : Changes to allow multiple ivectors per chunk

[freewym]

[vijayaditya]

I would recommend assuming ivector_period is 10 as default and modify all scripts and binaries according to that.

[vijayaditya]

I will do another round of review after these changes have been made.

[freewym]

Thanks! Done.

Yiming

[vijayaditya]

I am a bit busy this week and I don't want to delay this PR review, as it can be used to further improve chain results. @vimalmanohar or @pegahgh could you please help with the review ?

@freewym Would you please add to nnet3-chain-get-egs the changes you made to nnet3-get-egs ? If yes, these commits should be submitted to chain branch. Assume the current PR is going to be accepted to master and then merged to chain branch. So just make additional changes necessary.

[freewym]

Sure.

[vimalmanohar]

I am done with the review. It looks good for commit.

[danpovey]

@vijayaditya, as you can see I want a lot of changes, and some of them (e.g. involving descriptors) are not very trivial, so you might need to help Yiming a bit. There needs to be a lot more clarity in the decodable object. It seems Yiming was trying to make this change without making any structural changes to the code, and the result was very unclear.

[danpovey]

Also please make it so the function AmNnetSimple::Info() prints out the ivector-interval.

[vijayaditya]

OK, I will go through the required changes.

--Vijay

On Thu, Jan 28, 2016 at 4:50 PM, Daniel Povey [email protected] wrote:

Also please make it so the function AmNnetSimple::Info() prints out the ivector-interval.

— Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-176500779.

[danpovey]

Guys, is this pull request moving forward?

[vijayaditya]

I have to do the review. I will take care of it over the weekend.

--Vijay

On Sat, Feb 6, 2016 at 12:02 AM, Daniel Povey [email protected] wrote:

Guys, is this pull request moving forward?

— Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-180682037.

[freewym]

I have modified decodable code, and will further make changes according to Vijay's later review.

Yiming

On Saturday, February 6, 2016, Vijayaditya Peddinti < [email protected]> wrote:

I have to do the review. I will take care of it over the weekend.

--Vijay

On Sat, Feb 6, 2016 at 12:02 AM, Daniel Povey <[email protected] javascript:_e(%7B%7D,'cvml','[email protected]');> wrote:

Guys, is this pull request moving forward?

— Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-180682037.

— Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-180683271.

Sent from my iPhone

[danpovey]

@vijayaditya, what's the status of this? I'd like to get this finished, as it could improve results.

[vijayaditya]

Will be done this week.

On Tue, Feb 23, 2016 at 8:35 PM, Daniel Povey [email protected] wrote:

@vijayaditya https://github.com/vijayaditya, what's the status of this? I'd like to get this finished, as it could improve results.

— Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-188002289.

[vijayaditya]

Just an update: I have started this review, should be done by end of day.

[danpovey]

vijay, did you finish this review? I'd like to be able to merge this before we run our final experiments for the chain models- it should help.

[vijayaditya]

I was able to start the review yesterday. I am going through Yiming's suggested changes (offline) before I give my suggestions.

On Sat, Mar 5, 2016 at 12:23 AM, Daniel Povey [email protected] wrote:

vijay, did you finish this review? I'd like to be able to merge this before we run our final experiments for the chain models- it should help.

— Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-192580463.

[freewym]

@vijayaditya Changes have been made.

Yiming

[vijayaditya]

@freewym could you resolve the conflicts ? An easy way to do this would be to rebase your branch with the master.

[freewym]

Will do it later today

[vijayaditya]

@freewym what is the status of this PR? Is it ready for review ?

[freewym]

yes

On Monday, March 14, 2016, Vijayaditya Peddinti [email protected] wrote:

@freewym https://github.com/freewym what is the status of this PR? Is it ready for review ?

— Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-196245575.

Sent from my iPhone

[vijayaditya]

I am stopping the review. Will resume after you make required changes or justify why these changes are not necessary.

[freewym]

@danpovey could you please comment on the implementation of the function GetInputInterval() in nnet-utils.cc?

Yiming

[freewym]

Moved the comments in the braces. @vijayaditya I am done with the changes.

Thanks, Yiming

[vijayaditya]

OK will review in few hours.

On Tue, Mar 15, 2016 at 8:19 PM, Yiming Wang [email protected] wrote:

Moved the comments in the braces. @vijayaditya https://github.com/vijayaditya I am done with the changes.

Thanks, Yiming

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-197079424

[danpovey]

after code review we need to test it a bit before merge.

On Tue, Mar 15, 2016 at 8:20 PM, Vijayaditya Peddinti < [email protected]> wrote:

OK will review in few hours.

On Tue, Mar 15, 2016 at 8:19 PM, Yiming Wang [email protected] wrote:

Moved the comments in the braces. @vijayaditya https://github.com/vijayaditya I am done with the changes.

Thanks, Yiming

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-197079424

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-197079837

[vijayaditya]

@freewym I might not be able to review this till Saturday. In the mean time could you please add the unit tests for the decodable object and egs creation code with various descriptors combinations. (This would be the same thing you discussed offline today evening). Please try to keep these unit tests as random as possible, its up to you to decide the level of randomness.

[vijayaditya]

I was able to perform a cursory check. I think you should start an experiment and compare the performance with baseline (one ivector/chunk). You can create a new script based on swbd/s5c/local/chain/run_tdnn_6z.sh. You can make that script a part of this PR.

@danpovey I am assuming that we would not have time to repeat all the fshr+swbd experiments for the paper, with multiple ivectors. So I think getting _6z results on swbd, with multiple ivectors, should be sufficient for the immediate needs of the paper.

[danpovey]

On Mon, Mar 21, 2016 at 3:00 PM, Vijayaditya Peddinti < [email protected]> wrote:

I was able to perform a cursory check. I think you should start an experiment and compare the performance with baseline (one ivector/chunk). You can create a new script based on swbd/s5c/local/chain/run_tdnn_6z.sh. You can make that script a part of this PR.

@danpovey https://github.com/danpovey I am assuming that we would not have time to repeat all the fshr+swbd experiments for the paper, with multiple ivectors. So I think getting _6z results on swbd, with multiple ivectors, should be sufficient for the immediate needs of the paper.

• agreed.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/kaldi-asr/kaldi/pull/468#issuecomment-199425536