Minor changes required for nnet3 sequence training by vimalmanohar · Pull Request #560 · kaldi-asr/kaldi

vimalmanohar · 2016-03-06T02:39:45Z

No description provided.

danpovey · 2016-03-06T02:45:26Z

Thanks- will try to review soon. @vijayaditya, if you have time please take a look.

vijayaditya · 2016-03-06T03:19:54Z

OK, I will work on this after the ivector PR from Yiming.

--VIjay

On Sat, Mar 5, 2016 at 9:45 PM, Daniel Povey notifications@github.com
wrote:

Thanks- will try to review soon. @vijayaditya
https://github.com/vijayaditya, if you have time please take a look.

—
Reply to this email directly or view it on GitHub
#560 (comment).

vimalmanohar · 2016-03-06T04:24:34Z

Note: This is not the full set of changes. There is a lot of codes and scripts, which is beyond what the github diff allows to view. So I am committing them in small chunks that would be easier to look at. This is first set of commits.

vimalmanohar · 2016-03-06T04:25:25Z

I can push the next set of commits once this is merged.

danpovey · 2016-03-06T23:12:11Z

src/lat/lattice-functions.h

+// negated costs.  Requires that lat be topologically sorted.  This code
+// will work for either CompactLattice or Latice.
+template<typename LatticeType>
+double ComputeLatticeAlphasAndBetas(const LatticeType &lat,


vimal, it's not good for compilation speed to define this in the header.
Just declare it in the header, and in the .cc file, define it and instantiate it for float and double.

danpovey · 2016-03-07T00:04:26Z

OK, done. Please merge or rebase.

vimalmanohar · 2016-03-07T00:22:34Z

I have changed the functions to use SetUnderlyingLearningRate

danpovey · 2016-03-07T00:33:11Z

src/nnet3/nnet-utils.cc

+      if (uc == NULL)
+        KALDI_ERR << "Updatable component does not inherit from class "
+            "UpdatableComponent; change this code.";
+      uc->SetUnderlyingLearningRate(uc->LearningRate() * learning_rate_scale);


you actually want to use SetActualLearningRate() here.

vimalmanohar · 2016-03-07T01:15:23Z

I made the change.

danpovey · 2016-03-07T01:19:46Z

src/lat/lattice-functions.h

                                vector<double> *beta);

+
+static inline double LogAddOrMax(bool viterbi, double a, double b) {


do you still need this function to be in the header?

Its a small function that can be inlined. If it is moved to .cc file, it
cannot be inlined.

On Sun, Mar 6, 2016 at 8:19 PM Daniel Povey notifications@github.com
wrote:

In src/lat/lattice-functions.h
#560 (comment):

@@ -74,6 +74,26 @@ bool ComputeCompactLatticeAlphas(const CompactLattice &lat,
bool ComputeCompactLatticeBetas(const CompactLattice &lat,
vector *beta);

+static inline double LogAddOrMax(bool viterbi, double a, double b) {

do you still need this function to be in the header?

—
Reply to this email directly or view it on GitHub
https://github.com/kaldi-asr/kaldi/pull/560/files#r55153738.

Vimal Manohar
PhD Student
Electrical & Computer Engineering
Johns Hopkins University

so you are going to call it from some other code you have written?

also if you are going to leave it in the header you should take away the 'static'.

vimalmanohar · 2016-03-07T03:00:35Z

It seems there was only function that used it, which is not back in the .cc file. So I moved it to the .cc file.

danpovey · 2016-03-07T03:34:12Z

src/lat/lattice-functions.cc

  return true;
 }

+static inline double LogAddOrMax(bool viterbi, double a, double b) {


ok, you've now just moved this code around. could you revert this file?

danpovey · 2016-03-07T03:35:04Z

and please squash your commits.

vimalmanohar · 2016-03-07T03:44:09Z

Done

danpovey · 2016-03-07T03:47:23Z

src/nnet3/nnet-example-utils.h

+                             bool binary,
+                             Vector<BaseFloat> *vec);
+
+void RoundUpNumFrames(int32 frame_subsampling_factor,


you should document this declaration.

vimalmanohar · 2016-03-07T04:05:13Z

Fixed

danpovey · 2016-03-07T04:16:36Z

src/util/stl-utils.h

  }
 };

+/// Comparator object that does comparison on a different


This comparator is not very well explained.
However, I think I see what you are trying to do here-- it would be better, instead of using this comparator, to instead sort a vector of pairs, where the 1st of each pair is the thing you want to sort on, and the 2nd is the index that you want to sort.

This is where it is used:
https://github.com/vimalmanohar/kaldi/blob/sequence_clean/src/nnet3/discriminative-supervision.cc#L542
Would I have to write a similar comparator to sort the vector of pairs?

No you wouldn't have to write any comparator, the default one works.
I'm saying a make vector of pairs

(state-time, state-index)

and sort it. you don't even need stable_sort, since if state_time is the
same, the state_index gets used instead.

On Sun, Mar 6, 2016 at 11:27 PM, Vimal Manohar notifications@github.com
wrote:

In src/util/stl-utils.h
#560 (comment):

@@ -292,6 +292,29 @@ struct CompareFirstMemberOfPair {
}
};

+/// Comparator object that does comparison on a different

This is where it is used:

https://github.com/vimalmanohar/kaldi/blob/sequence_clean/src/nnet3/discriminative-supervision.cc#L542
Would I have to write a similar comparator to sort the vector of pairs?

—
Reply to this email directly or view it on GitHub
https://github.com/kaldi-asr/kaldi/pull/560/files#r55161297.

Ok, I removed the comparator from stl-utils.h. I will modify the sort code
to use pairs.

On Sun, Mar 6, 2016 at 11:36 PM Daniel Povey notifications@github.com
wrote:

In src/util/stl-utils.h
#560 (comment):

@@ -292,6 +292,29 @@ struct CompareFirstMemberOfPair {
}
};

+/// Comparator object that does comparison on a different

No you wouldn't have to write any comparator, the default one works. I'm
saying a make vector of pairs (state-time, state-index) and sort it. you
don't even need stable_sort, since if state_time is the same, the
state_index gets used instead.
… <#msg-f:1528116455082592170_>
On Sun, Mar 6, 2016 at 11:27 PM, Vimal Manohar notifications@github.com
wrote: In src/util/stl-utils.h <#560 (comment)
https://github.com/kaldi-asr/kaldi/pull/560#discussion_r55161297>: > @@
-292,6 +292,29 @@ struct CompareFirstMemberOfPair { > } > }; > > +///
Comparator object that does comparison on a different This is where it is
used:
https://github.com/vimalmanohar/kaldi/blob/sequence_clean/src/nnet3/discriminative-supervision.cc#L542
Would I have to write a similar comparator to sort the vector of pairs? —
Reply to this email directly or view it on GitHub <
https://github.com/kaldi-asr/kaldi/pull/560/files#r55161297>.

—
Reply to this email directly or view it on GitHub
https://github.com/kaldi-asr/kaldi/pull/560/files#r55161622.

Vimal Manohar
PhD Student
Electrical & Computer Engineering
Johns Hopkins University

danpovey · 2016-03-07T04:47:45Z

OK, thanks! Merging.

Minor changes required for nnet3 sequence training

danpovey reviewed Mar 6, 2016
View reviewed changes

vimalmanohar force-pushed the sequence_changes branch 2 times, most recently from cb187d9 to 541e4ae Compare March 7, 2016 00:20

danpovey reviewed Mar 7, 2016
View reviewed changes

vimalmanohar force-pushed the sequence_changes branch from 888ddc2 to 5bd5fb4 Compare March 7, 2016 02:58

danpovey reviewed Mar 7, 2016
View reviewed changes

vimalmanohar force-pushed the sequence_changes branch from 5bd5fb4 to 10fbf54 Compare March 7, 2016 03:43

danpovey reviewed Mar 7, 2016
View reviewed changes

vimalmanohar force-pushed the sequence_changes branch from 10fbf54 to cf7a66b Compare March 7, 2016 04:04

danpovey reviewed Mar 7, 2016
View reviewed changes

sequence: Minor changes required for sequence training

e0e2065

vimalmanohar force-pushed the sequence_changes branch from cf7a66b to e0e2065 Compare March 7, 2016 04:43

danpovey added a commit that referenced this pull request Mar 7, 2016

Merge pull request #560 from vimalmanohar/sequence_changes

2c9bff8

Minor changes required for nnet3 sequence training

danpovey merged commit 2c9bff8 into kaldi-asr:chain Mar 7, 2016

		vector<double> *beta);


		static inline double LogAddOrMax(bool viterbi, double a, double b) {

Conversation

vimalmanohar commented Mar 6, 2016

Uh oh!

danpovey commented Mar 6, 2016

Uh oh!

vijayaditya commented Mar 6, 2016

Uh oh!

vimalmanohar commented Mar 6, 2016

Uh oh!

vimalmanohar commented Mar 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danpovey commented Mar 7, 2016

Uh oh!

vimalmanohar commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vimalmanohar commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vimalmanohar commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danpovey commented Mar 7, 2016

Uh oh!

vimalmanohar commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vimalmanohar commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danpovey commented Mar 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants