nnet3-rnnlm lattice rescoring draft #1906

hainan-xv · 2017-09-24T18:49:06Z

No description provided.

danpovey · 2017-09-24T19:01:39Z

egs/wsj/s5/steps/lmrescore_rnnlm_lat.sh

@@ -61,6 +61,11 @@ if [ "$rnnlm_ver" == "tensorflow" ]; then
  first_arg="$rnnlm_dir/unk.probs $rnnlm_dir/wordlist.rnn.final"
 fi

+if [ "$rnnlm_ver" == "kaldirnnlm" ]; then


I think it might be better at this point to have a separate script for this type of lattice rescoring, located in scripts/rnnlm/. That will keep things separate and will make it easier to refactor in future.

danpovey

A bunch of high-level comments.

danpovey · 2017-09-24T19:06:56Z

scripts/rnnlm/train_rnnlm.sh

 fi

+touch $dir/unk.probs


once we modify this setup to have its own rescoring scripts, unk.probs may no longer be needed. but I may merge this as-is for now.

danpovey · 2017-09-24T19:09:24Z

src/latbin/Makefile

@@ -21,18 +24,20 @@ BINFILES = lattice-best-path lattice-prune lattice-equivalent lattice-to-nbest \
           lattice-confidence lattice-determinize-phone-pruned \
           lattice-determinize-phone-pruned-parallel lattice-expand-ngram \
           lattice-lmrescore-const-arpa lattice-lmrescore-rnnlm nbest-to-prons \
-           lattice-arc-post lattice-determinize-non-compact \
+           lattice-arc-post lattice-determinize-non-compact lattice-lmrescore-kaldi-rnnlm \


If the "latbin" directory now depends on nnet3 and cudamatrix, you will have to update the directory-level dependencies in ../Makefile.

You could also see what misc/maintenance/reorder_addlibs.sh and misc/maintenance/find_missing_dependencies.sh do-- it may not be necessary to do this manually.
Those tools haven't been run in a while and they may try to fix unrelated errors.

danpovey · 2017-09-24T19:09:45Z

src/latbin/lattice-lmrescore-kaldi-rnnlm.cc

+// latbin/lattice-lmrescore-kaldi-rnnlm.cc
+
+// Copyright 2017 Johns Hopkins University (author: Daniel Povey)
+//                Hainan Xu


should have date on each line, e.g.
2017 Hainan Xu

danpovey · 2017-09-24T19:10:40Z

src/rnnlm/rnnlm-decodable-simple-looped.cc

+
+  int32 ivector_period = frames_per_chunk;
+  int32 extra_right_context = 0;
+  int32 num_sequences = 1;  // we're processing one utterance at a time.


one utterance -> one word-sequence

danpovey · 2017-09-24T19:11:39Z

src/rnnlm/rnnlm-decodable-simple-looped.cc

+                                      frame - current_log_post_offset_);
+  BaseFloat log_prob =
+    VecVec(hidden, word_embedding_mat.Row(word_index));
+//      output_layer->ComputeLogprobOfWordGivenHistory(hidden, word_index);


it would probably make sense to have an option here to do the correct normalization, just for baseline-ing purposes if nothing else.

danpovey · 2017-09-24T19:12:16Z

src/rnnlm/rnnlm-decodable-simple-looped.h

+// IsSimpleNnet(nnet) would return true.  'looped' means we use looped
+// computations, with a kGotoLabel statement at the end of it.
+struct DecodableRnnlmSimpleLoopedComputationOptions {
+  int32 extra_left_context_initial;


I don't think we need the extra-left-context-initial. We don't anticipate using it in this setup.

danpovey · 2017-09-24T19:14:24Z

src/rnnlm/rnnlm-decodable-simple-looped.h

+  // frames.  If you deviate from this, one of these calls may crash.
+  void GetNnetOutputForFrame(int32 frame, VectorBase<BaseFloat> *output);
+
+  // Updates feats_ with the new incoming word specified in word_indexes


mention that we'll normally supply these one at a time.

danpovey · 2017-09-24T19:15:05Z

src/rnnlm/rnnlm-decodable-simple-looped.h

+  via other wrapper classes.
+
+  It accept just input features */
+class DecodableRnnlmSimpleLooped {


I don't think it's necessary to call this Decodable. This isn't used in any context that requires the Decodable interface, is it?

danpovey · 2017-09-24T19:15:51Z

src/rnnlm/rnnlm-decodable-simple-looped.h

+                        this class needs, and contains a pointer to the neural net.
+     @param [in] feats  The input feature matrix.
+  */
+  DecodableRnnlmSimpleLooped(const DecodableRnnlmSimpleLoopedInfo &info);


I'm surprised that this class doesn't have a copy constructor. Or are you using the default? If so you should clarify so (e.g. using
DecodableRnnlmSimpleLooped(const DecodableRnnlmSimpleLooped &other) = default;
if allowed).

I don't understand how you handle branching of paths if there is no copy constructor.

danpovey · 2017-09-24T19:17:17Z

src/rnnlm/rnnlm-decodable-simple-looped.h

+  DecodableRnnlmSimpleLooped(const DecodableRnnlmSimpleLoopedInfo &info);
+
+  // returns the number of frames of likelihoods.  The same as feats_.NumRows()
+  inline int32 NumFrames() const { return num_frames_; }


I think it might be better to give this function (if it is even needed) a name that's more meaningful for the current context-- no need to stick to the DecodableInterface. Reconsider the names of other member functions too.

danpovey

A few more comments.

danpovey · 2017-09-24T19:45:12Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+  KALDI_ASSERT(static_cast<size_t>(s) < state_to_wseq_.size());
+
+  std::vector<Label> wseq = state_to_wseq_[s];
+  DecodableRnnlmSimpleLooped decodable_rnnlm = state_to_decodable_rnnlm_[s];


It looks like you are implicitly using the copy constructor of this class. You should make this explicit by writing out the copy constructor (and also make the assignment operator private to disallow its use).

However, there are some problems with how this is written. When the storage of state_to_decodable_rnnlm_ is reallocated, expensive copies and destructions of the DecodableRnnlmSimpleLooped objects will take place. state_to_decodable_rnnlm_[s] should instead contain pointers. And you should allocate these objects via "new", instead of on the stack. For non-initial states, your "new" operation can use the self-constructor. The issue with allocating this on the stack is that you have to copy and then destroy this object, which is very wasteful.

danpovey · 2017-09-24T19:46:56Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+
+bool KaldiRnnlmDeterministicFst::GetArc(StateId s, Label ilabel,
+                                        fst::StdArc *oarc) {
+  // At this point, we should have created the state.


You should be caching the arcs here, to avoid wasteful invocations of the RNNLM computation. If you don't want to do this explicitly in this class, I believe by wrapping this in a CacheDeterministicOnDemandFst you can get that for free (it would require a modification in the calling code).

danpovey · 2017-09-24T19:48:00Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+  ReadFstWordSymbolTableAndRnnWordlist(rnn_wordlist,
+                                       word_symbol_table_rxfilename);
+
+  std::vector<Label> bos;


probably better to rename bos -> bos_seq

danpovey · 2017-09-24T19:49:41Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+  bos.push_back(0); // 0 for <s>
+  state_to_wseq_.push_back(bos);
+  DecodableRnnlmSimpleLooped decodable_rnnlm(info);
+  decodable_rnnlm.TakeFeatures(std::vector<Label>(1, bos[0]));


It looks to me like you are just calling TakeFeatures with a copy of the 'bos' vector, constructed in a rather strange way. Why not use bos (rename this to bos_seq) directly?

danpovey

Some more comments

danpovey · 2017-09-27T00:49:29Z

src/rnnlm/rnnlm-lattice-rescoring.h

+  std::vector<std::vector<Label> > state_to_wseq_;
+
+  // mapping from state-id to RNNLM structure including computer
+  std::vector<RnnlmSimpleLooped*> state_to_decodable_rnnlm_;


This is now not a good name for this variable. Maybe state_to_rnnlm_state_.

danpovey · 2017-09-27T00:52:09Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+  if (result.second == true) {
+    RnnlmSimpleLooped *rnnlm2 = new RnnlmSimpleLooped(*rnnlm);  // make a copy
+
+//  std::vector<int32> fst_label_to_rnn_in_label_;


remove these comments.

danpovey · 2017-09-27T00:53:14Z

src/rnnlm/rnnlm-lattice-rescoring.h

+  // mapping from state-id to RNNLM structure including computer
+  std::vector<RnnlmSimpleLooped*> state_to_decodable_rnnlm_;
+
+  // mapping from state-id to output word-embedding


clarify whether these pointers areowned here.

danpovey · 2017-09-27T01:04:17Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+    string word;
+    int32 i = 0;
+    while (ifile >> word >> id) {
+      if (word == "</s>") {


IIRC, other parts of the code don't assume that these are the written forms of BOS and EOS; they take these as options. The scripts do, currently, so this might not be fatal-- but generally speaking, programs don't take any arguments with written forms of words unless they absolutely have to for some reason, and this program breaks that pattern. I'd be happier to add the same options as for the training tools, with --bos-symbol and --eos-symbol (and an ignored option, --brk-symbol, if it makes your life easier scripting-wise).

danpovey · 2017-09-27T01:05:32Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+      rnn_label_to_word_.push_back(word);
+
+      int fst_label = fst_word_symbols->Find(rnn_label_to_word_[id]);
+      if (fst::SymbolTable::kNoSymbol != fst_label || id == out_OOS_index_ || id == 0 || word == "<brk>") {}


please clean up this code, it's weird.

danpovey · 2017-09-27T01:07:36Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+      else {
+        KALDI_LOG << "warning: word " << word << " in RNNLM wordlist but not in FST wordlist";
+      }
+      if (id != out_OOS_index_ && out_OOS_index_ != 0 && fst_label != -1) {


Capitals should not be in variables like this.
And I think it's strange that you are implementing this mapping of symbols; I think it would be best to require that they are the same. It's easy to generate an embedding matrix w.r.t. an arbitrary wordlist if you are using the letter features; and if not, it wouldn't be that hard to write a script to mess with the row-indexes of the word-embedding matrix. Dealing with the strings like this in the code is rather messy.

danpovey · 2017-09-27T01:12:01Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+//  std::vector<std::string> rnn_in_label_to_word_;
+    rnnlm2->TakeFeatures(std::vector<Label>(1, rnn_word));
+    state_to_wseq_.push_back(wseq);
+    state_to_nnet3_output_.push_back(rnnlm2->GetOutput(0));


I don't understand why this is zero here.
Either this is a bug, or your interfaces don't make sense.

danpovey

.. and some more comments.

danpovey · 2017-09-27T01:14:07Z

src/rnnlm/rnnlm-simple-looped.h

+
+  inline int32 NnetOutputDim() const { return info_.nnet_output_dim; }
+
+  // Gets the nnet's output for a particular frame, with 0 <= frame < NumFrames().


remove this old code.

danpovey · 2017-09-27T01:14:27Z

src/rnnlm/rnnlm-simple-looped.h

+  void TakeFeatures(const std::vector<int32> &word_indexes);
+
+  // Gets the output for a particular frame and word_index, with
+  // 0 <= frame < NumFrames().


remove old comments.

danpovey · 2017-09-27T01:14:50Z

src/rnnlm/rnnlm-simple-looped.h

+  // Gets the output for a particular frame and word_index, with
+  // 0 <= frame < NumFrames().
+//  BaseFloat GetOutput(int32 frame, int32 word_index);
+  // create a CuVector in heap, pointer owned by the caller


You need to clarify this interface. You don't explain what this output or how the frame relates to positions in the word sequence.

danpovey · 2017-09-27T01:16:40Z

src/rnnlm/rnnlm-simple-looped.cc

+
+void RnnlmSimpleLooped::AdvanceChunk() {
+  int32 begin_input_frame, end_input_frame;
+  begin_input_frame = -info_.frames_left_context;


the frames_left_context and frames_rigth_context will be zero, this code can be simplified a lot.

danpovey · 2017-09-27T01:17:18Z

src/rnnlm/rnnlm-simple-looped.cc

+  // currently there is no left/right context and frames_per_chunk == 1
+  KALDI_ASSERT(begin_input_frame == 0 && end_input_frame == 1);
+
+  SparseMatrix<BaseFloat> feats_chunk(end_input_frame - begin_input_frame,


this code seems to have been copied from code designed for an ASR application without much thought to properly designing it for this case.

danpovey · 2017-09-27T01:19:05Z

src/rnnlm/rnnlm-simple-looped.cc

+  KALDI_ASSERT(current_nnet_output_.NumRows() == info_.frames_per_chunk &&
+               current_nnet_output_.NumCols() == info_.nnet_output_dim);
+
+  current_log_post_offset_ = 0;


This doesn't make sense. If the variable is always zero, why have the variable at all?

danpovey · 2017-09-27T01:27:42Z

src/rnnlm/rnnlm-simple-looped.h

+
+  // Updates feats_ with the new incoming word specified in word_indexes
+  // We usually do this one at a time
+  void TakeFeatures(const std::vector<int32> &word_indexes);


You always advance this one word at a time, so I think it would make more sense to have an interface like

/// Advance the state of the RNNLM by appending this word to the word sequence. void AddWord(int32 word); /// Return the log-prob that the model predicts for the provided word-index, /// given the previous history determined by the sequence of calls to AddWord() /// (implicitly starting with the BOS symbol). BaseFloat LogProbOfWord(int32 word_index) const;

The class itself can internally maintain a pointer (maybe just a BaseFloat* pointing to GPU memory)
to the vector consisting of the neural network's output at the current position. This can be a pointer
into the matrix of output obtained by calling GetOutput() with the non-destructive version.
There is no need to expose the vector that the neural network outputs.

danpovey · 2017-09-27T01:29:00Z

src/rnnlm/rnnlm-simple-looped.h

+
+  NnetComputer computer_;
+
+  SparseMatrix<BaseFloat> feats_;


There is no reason to have any SparseMatrix in this class. Each time you process a new word you just look up the vector and provide it as input. You need to forget how the code worked for the regular decodable object.

danpovey · 2017-09-27T01:30:06Z

src/rnnlm/rnnlm-simple-looped.h

+
+  // The current nnet's output that we got from the last time we
+  // ran the computation.
+  Matrix<BaseFloat> current_nnet_output_;


... and there no need to store the nnet output in CPU memory. That was done in the decoding case because single-element lookups are slow to do one by one to GPU. But if we are using a GPU, we'll want to do the vector dot-products on GPU.

danpovey · 2017-09-27T19:03:03Z

src/rnnlm/rnnlm-simple-looped.cc

+  BaseFloat log_prob;
+
+  if (info_.opts.force_normalize) {
+    CuVector<BaseFloat> log_probs(word_embedding_mat.NumRows());


if you are going to normalize, you should do this just once when you create the state, not each time you process an output arc.
[c.f. LogSumExp() function or similar.]

hainan-xv · 2017-09-27T19:10:24Z

Hmm, I guess you mean I should have a state_to_normalization_term_ vector and store the sum in it the first time it is computed?

…

On Wed, Sep 27, 2017 at 3:03 PM, Daniel Povey ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/rnnlm/rnnlm-simple-looped.cc <#1906 (comment)>: > +//void RnnlmSimpleLooped::GetNnetOutputForFrame( +// int32 frame, VectorBase<BaseFloat> *output) { +// KALDI_ASSERT(frame >= 0 && frame < feats_.NumRows()); +// if (frame >= current_log_post_offset_ + current_nnet_output_.NumRows()) +// AdvanceChunk(); +// output->CopyFromVec(current_nnet_output_.Row(frame - +// current_log_post_offset_)); +//} + +BaseFloat RnnlmSimpleLooped::LogProbOfWord(int32 word_index, + const CuVectorBase<BaseFloat> &hidden) const { + const CuMatrix<BaseFloat> &word_embedding_mat = info_.word_embedding_mat; + BaseFloat log_prob; + + if (info_.opts.force_normalize) { + CuVector<BaseFloat> log_probs(word_embedding_mat.NumRows()); if you are going to normalize, you should do this just once when you create the state, not each time you process an output arc. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1906 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFMCDli1q9PpRnR4H878-JvXA4F6fNf_ks5smpwCgaJpZM4Ph_FH> .

-- - Hainan

danpovey

Looking for the bug; didn't find it but found a bunch of issues.

danpovey · 2017-09-27T19:05:24Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+  std::pair<const std::vector<Label>, StateId> wseq_state_pair(
+      wseq, static_cast<Label>(state_to_wseq_.size()));
+
+  // Attemps to insert the current <lseq_state_pair>. If the pair already exists


lseq should be wseq I think. but wseq is a too-short acronym.

danpovey · 2017-09-27T19:07:11Z

src/rnnlm/rnnlm-simple-looped.h

+                        this class needs, and contains a pointer to the neural net.
+     @param [in] feats  The input feature matrix.
+  */
+  RnnlmSimpleLooped(const RnnlmSimpleLoopedInfo &info);


The name of this is quite non-descriptive. Maybe RnnlmLoopedComputer.

danpovey · 2017-09-27T19:07:41Z

src/rnnlm/rnnlm-simple-looped.h

+  int32 frames_right_context;
+  // The frames_per_chunk equals the number of input frames we need for each
+  // chunk (except for the first chunk).
+  int32 frames_per_chunk;


frames_per_chunk will always be 1 so remove it; also frames_left_context and frames_right_context will always be 0 for now so you can remove them.

danpovey · 2017-09-27T19:08:39Z

src/rnnlm/rnnlm-simple-looped.h

+  // The 3 computation requests that are used to create the looped
+  // computation are stored in the class, as we need them to work out
+  // exactly shich iVectors are needed.
+  ComputationRequest request1, request2, request3;


remove comments about ivectors. And request1 through request3 should be local (stack) variables, they are not needed in this class any more.

danpovey · 2017-09-27T19:08:56Z

src/rnnlm/rnnlm-simple-looped.h

+  int32 frames_per_chunk;
+
+  // The output dimension of the nnet neural network (not the final output).
+  int32 nnet_output_dim;


nnet_output_dim should be renamed to embedding_dim.

danpovey · 2017-09-27T19:11:30Z

src/rnnlm/rnnlm-simple-looped.cc

+//  int32 num_words = info_.word_embedding_mat.NumRows();
+
+
+  CuMatrix<BaseFloat> current_nnet_output_gpu;


this swapping back and forth between CPU and GPU is wasteful- just keep it on GPU, but no need to copy at all- you can use the non-destructive GetOutput function and maintain a pointer to within the computation itself.

danpovey · 2017-09-27T19:12:44Z

src/rnnlm/rnnlm-simple-looped.h

+                   "by the neural net.");
+    opts->Register("debug-computation", &debug_computation, "If true, turn on "
+                   "debug for the actual computation (very verbose!)");
+    opts->Register("force-normalize", &force_normalize, "If true, force "


this explanation is not very clear IMO. Maybe
"If true, word probabilities will be correctly normalized (otherwise the sum-to-one normalization
is approximate)"
I recommend to rename the option to "--normalize-probs".

danpovey · 2017-09-27T19:13:40Z

src/rnnlm/rnnlm-simple-looped.h

+      const kaldi::nnet3::Nnet &rnnlm,
+      const CuMatrix<BaseFloat> &word_embedding_mat);
+
+  void Init(const RnnlmSimpleLoopedComputationOptions &opts,


I don't think this Int funtion needs to be public; in fact, I think it could all be included in the constructor.

danpovey · 2017-09-27T19:14:21Z

src/rnnlm/rnnlm-simple-looped.h

+class RnnlmSimpleLoopedInfo  {
+ public:
+  RnnlmSimpleLoopedInfo(
+      const RnnlmSimpleLoopedComputationOptions &opts,


Clarify, that it retains references to these 3 things (assuming it does); and explain in a comment what this class is for.

danpovey · 2017-09-27T19:14:35Z

src/rnnlm/rnnlm-simple-looped.h

+class RnnlmSimpleLooped {
+ public:
+  /**
+     This constructor takes features as input.


this comment seems to be out of date.

danpovey · 2017-09-27T19:24:30Z

No I mean it would be stored inside the object you are currently calling RnnlmSimpleLooped (which you should probably rename and rewrite from scratch). That already has all the state you need; there is no need to copy state to any external location.

…

On Wed, Sep 27, 2017 at 3:10 PM, hainan-xv ***@***.***> wrote: Hmm, I guess you mean I should have a state_to_normalization_term_ vector and store the sum in it the first time it is computed? On Wed, Sep 27, 2017 at 3:03 PM, Daniel Povey ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/rnnlm/rnnlm-simple-looped.cc > <#1906 (comment)>: > > > +//void RnnlmSimpleLooped::GetNnetOutputForFrame( > +// int32 frame, VectorBase<BaseFloat> *output) { > +// KALDI_ASSERT(frame >= 0 && frame < feats_.NumRows()); > +// if (frame >= current_log_post_offset_ + current_nnet_output_.NumRows()) > +// AdvanceChunk(); > +// output->CopyFromVec(current_nnet_output_.Row(frame - > +// current_log_post_offset_)); > +//} > + > +BaseFloat RnnlmSimpleLooped::LogProbOfWord(int32 word_index, > + const CuVectorBase<BaseFloat> &hidden) const { > + const CuMatrix<BaseFloat> &word_embedding_mat = info_.word_embedding_mat; > + BaseFloat log_prob; > + > + if (info_.opts.force_normalize) { > + CuVector<BaseFloat> log_probs(word_embedding_mat.NumRows()); > > if you are going to normalize, you should do this just once when you > create the state, not each time you process an output arc. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1906 (review) >, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AFMCDli1q9PpRnR4H878- JvXA4F6fNf_ks5smpwCgaJpZM4Ph_FH> > . > -- - Hainan — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1906 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVuxAtFlyx5AcbP8CRUjheBgOp0bYmks5smp2qgaJpZM4Ph_FH> .

danpovey · 2017-09-27T19:29:41Z

... actually instead of using the self-constructor, it would probably make more sense to have a function `RnnlmSimpleLooped *GetSuccessorState(int32 next_word) const;` which gives you the object corresponding to the successor state. Maybe the class could be called RnnlmComputeState.

…

On Wed, Sep 27, 2017 at 3:24 PM, Daniel Povey ***@***.***> wrote: No I mean it would be stored inside the object you are currently calling RnnlmSimpleLooped (which you should probably rename and rewrite from scratch). That already has all the state you need; there is no need to copy state to any external location. On Wed, Sep 27, 2017 at 3:10 PM, hainan-xv ***@***.***> wrote: > Hmm, I guess you mean I should have a > > state_to_normalization_term_ > > vector and store the sum in it the first time it is computed? > > On Wed, Sep 27, 2017 at 3:03 PM, Daniel Povey ***@***.***> > wrote: > > > ***@***.**** commented on this pull request. > > ------------------------------ > > > > In src/rnnlm/rnnlm-simple-looped.cc > > <#1906 (comment)>: > > > > > +//void RnnlmSimpleLooped::GetNnetOutputForFrame( > > +// int32 frame, VectorBase<BaseFloat> *output) { > > +// KALDI_ASSERT(frame >= 0 && frame < feats_.NumRows()); > > +// if (frame >= current_log_post_offset_ + > current_nnet_output_.NumRows()) > > +// AdvanceChunk(); > > +// output->CopyFromVec(current_nnet_output_.Row(frame - > > +// current_log_post_offset_)); > > +//} > > + > > +BaseFloat RnnlmSimpleLooped::LogProbOfWord(int32 word_index, > > + const CuVectorBase<BaseFloat> &hidden) const { > > + const CuMatrix<BaseFloat> &word_embedding_mat = > info_.word_embedding_mat; > > + BaseFloat log_prob; > > + > > + if (info_.opts.force_normalize) { > > + CuVector<BaseFloat> log_probs(word_embedding_mat.NumRows()); > > > > if you are going to normalize, you should do this just once when you > > create the state, not each time you process an output arc. > > > > — > > You are receiving this because you authored the thread. > > Reply to this email directly, view it on GitHub > > <#1906 (comment) > iew-65652209>, > > or mute the thread > > <https://github.com/notifications/unsubscribe-auth/ > AFMCDli1q9PpRnR4H878-JvXA4F6fNf_ks5smpwCgaJpZM4Ph_FH> > > . > > > > > > -- > - Hainan > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#1906 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ADJVuxAtFlyx5AcbP8CRUjheBgOp0bYmks5smp2qgaJpZM4Ph_FH> > . >

danpovey · 2017-09-30T17:09:09Z

src/rnnlm/rnnlm-simple-looped.h

+  void Register(OptionsItf *opts) {
+    opts->Register("frames-per-chunk", &frames_per_chunk,
+                   "Number of frames in each chunk that is separately evaluated "
+                   "by the neural net.");


it's not clear to me that it makes sense to make frames_per_chunk configurable. The setup wouldn't work
if this were set to a value different from 1.

hainan-xv · 2017-09-30T18:04:12Z

Hmm this is a rather old part of the PR for which I have already re-written. I believe it's not there in the latest commit. I will have a ready PR by tonight.

…

On Sat, Sep 30, 2017 at 1:09 PM, Daniel Povey ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/rnnlm/rnnlm-simple-looped.h <#1906 (comment)>: > + bool force_normalize; + NnetOptimizeOptions optimize_config; + NnetComputeOptions compute_config; + RnnlmSimpleLoopedComputationOptions(): + frames_per_chunk(1), + debug_computation(false), + force_normalize(false) { } + + void Check() const { + KALDI_ASSERT(frames_per_chunk > 0); + } + + void Register(OptionsItf *opts) { + opts->Register("frames-per-chunk", &frames_per_chunk, + "Number of frames in each chunk that is separately evaluated " + "by the neural net."); it's not clear to me that it makes sense to make frames_per_chunk configurable. The setup wouldn't work if this were set to a value different from 1. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1906 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFMCDhe85cA3GagGBhmlBrVGEH3GcFkHks5snnXBgaJpZM4Ph_FH> .

-- - Hainan

…ption

danpovey

These comments are now very minor.
But I think this must not contain your latest changes, e.g. with the pruned composition.

danpovey · 2017-10-06T19:30:32Z

scripts/rnnlm/train_rnnlm.sh


 if [ -f $dir/feat_embedding.0.mat ]; then
  sparse_features=true
+  embedding_type=feat_embedding


let's just make this either "feat" or "word", remove the "_embedding".

danpovey · 2017-10-06T20:09:21Z

src/latbin/lattice-lmrescore-kaldi-rnnlm.cc

+    typedef kaldi::int64 int64;
+
+    const char *usage =
+        "Rescores lattice with rnnlm. The LM will be wrapped into the\n"


This documentation is too code-level. Remove most of this text, and instead just mention the script in steps/ that calls it.

danpovey · 2017-10-06T20:10:06Z

src/latbin/lattice-lmrescore-kaldi-rnnlm.cc

+    po.Register("lm-scale", &lm_scale, "Scaling factor for language model "
+                "costs; frequently 1.0 or -1.0");
+    po.Register("max-ngram-order", &max_ngram_order, "If positive, limit the "
+                "rnnlm context to the given number, -1 means we are not going "


this is not quite accurate, we don't limit, we identify contexts that are equal in the n most recent words.

danpovey · 2017-10-06T20:16:42Z

scripts/rnnlm/prepare_rnnlm_dir.sh

@@ -157,13 +157,16 @@ if [ $stage -le 7 ]; then
    num_splits=$(cat $dir/text/info/num_splits)
    text_files=$(for n in $(seq $num_splits); do echo -n $dir/text/$n.txt ''; done)
    vocab_size=$(tail -n 1 $dir/config/words.txt | awk '{print $NF + 1}')
+    bos_symbol=`grep "<s>" $dir/config/words.txt | awk '{print $2}'`


see if you can avoid adding lines like this, it's not very robust (what if a word contains this?).

See $dir/special_symbol_opts.txt. You may be able to get the options from there.

danpovey · 2017-10-11T04:16:38Z

egs/swbd/s5/local/rnnlm/run_lstm_d.sh

@@ -0,0 +1,88 @@
+#!/bin/bash
+


I think you should introduce the 'tuning' directory concept and introduce soft links to the best current model. Otherwise this won't scale well.

danpovey

I want to review more of the C++ code to-morrow, but here are some initial comments.

danpovey · 2017-10-11T04:18:00Z

egs/swbd/s5/local/rnnlm/run_lstm_d.sh

+# This script takes no command-line arguments but takes the --cmd option.
+
+# Begin configuration section.
+cmd=run.pl


this is not right. You should be sourcing cmd.sh and using e.g. train_cmd from there.
But I notice later in the script you are not even using this, you use queue.pl directly.

danpovey · 2017-10-11T04:19:04Z

egs/swbd/s5/local/rnnlm/run_lstm_d.sh

+if [ $stage -le 2 ]; then
+  # the --unigram-factor option is set larger than the default (100)
+  # in order to reduce the size of the sampling LM, because rnnlm-get-egs
+  # was taking up too much CPU (as much as 10 cores).


can you check if this is still the case (i.e. without this is it still taking up too many cores?)
Because I fixed an issue that was causing it to use too many cores (it was spinning on locks).

Just ran this with the default 100 and it's not using more than one core.

Deleting the comment. Should I change the 200 to the default 100?

You can remove the option.

danpovey · 2017-10-11T04:21:27Z

egs/swbd/s5/local/rnnlm/run_rescoring.sh

@@ -0,0 +1,26 @@
+#!/bin/bash
+


I'm not sure that I like the fact that the training script and rescoring script are separate.
It means that after you run the training it may not be clear how to run the rescoring.
IMO at the very least you should say, at the top of the training script, how to test it via
rescoring.

danpovey · 2017-10-11T04:27:56Z

egs/swbd/s5/local/score.sh

@@ -31,7 +31,7 @@ data=$1

 if [ -f $data/stm ]; then # use sclite scoring.
  echo "$data/stm exists: using local/score_sclite.sh"
-  eval local/score_sclite.sh $orig_args
+  eval local/score_sclite.sh "$orig_args"


I just noticed that all of these changes are in swbd/s5. This is super outdated. You should be using s5c. I doubt that this problem (if there was a problem) occurs in the latest script. In any case let me know what the problem was, because I'd be surprised if this was really a bug, this script being so old.

IIRC if I don't add the "", if the $orig_args has something like --cmd "queue.pl --mem 8G" it'll complain.

danpovey · 2017-10-11T04:29:06Z

scripts/rnnlm/get_word_features.py

@@ -132,6 +142,12 @@ def read_features(features_file):
    unigram_probs = None
 feats = read_features(args.features_file)

+def treat_as_bos(word):


This is super inefficient. Figure out a better way to do this, e.g. create a set 'treat_as_bos_set' and do if word in treat_as_bos_set:.

danpovey · 2017-10-11T04:36:33Z

scripts/rnnlm/lmrescore_rnnlm_lat.sh

+nj=`cat $indir/num_jobs` || exit 1;
+cp $indir/num_jobs $outdir
+
+oldlm_weight=`perl -e "print -1.0 * $weight;"`


I generally prefer $() to backticks. Not all of the older scripts follow this.

danpovey · 2017-10-11T04:42:40Z

scripts/rnnlm/lmrescore_rnnlm_lat.sh

+    lattice-lmrescore-const-arpa --lm-scale=$oldlm_weight \
+    "ark:gunzip -c $indir/lat.JOB.gz|" "$oldlm" ark:-  \| \
+    lattice-lmrescore-kaldi-rnnlm --lm-scale=$weight \
+    --bos-symbol=$bos_symbol --eos-symbol=$eos_symbol \


don't you mean special_symbol_opts?

Also I am not sure that I like the name lmrescore_rnnlm_lat.sh, because I think this script might become deprecated when we introduce the pruned rescoring code that does everything in one program (wasn't that better)... so maybe it should have the longer name and the pruned one should have the simpler name. Or maybe it could be an option in this script, in which case I guess the name is OK. Also, maybe the "lat" isn't needed in the name because things are implicitly lattices, and if it was n-best we could put "nbest" in the name.

danpovey · 2017-10-11T04:43:12Z

scripts/rnnlm/prepare_rnnlm_dir.sh

@@ -158,12 +158,14 @@ if [ $stage -le 7 ]; then
    text_files=$(for n in $(seq $num_splits); do echo -n $dir/text/$n.txt ''; done)
    vocab_size=$(tail -n 1 $dir/config/words.txt | awk '{print $NF + 1}')

+    special_symbol_opts=`cat $dir/special_symbol_opts.txt | sed "s= =\n=g" | egrep "bos|eos" | tr "\n" " "`


when you address my previous comment, make sure this line gets changed too.

danpovey · 2017-10-11T04:44:27Z

src/latbin/lattice-lmrescore-kaldi-rnnlm.cc

+    }
+
+    if (opts.bos_index == -1 || opts.eos_index == -1) {
+      KALDI_ERR << "must set --bos-symbol and --eos-symbol options";


error messages should be properly capitalized and be proper sentences. Change to "You must set the " at the beginning.

danpovey · 2017-10-11T04:44:48Z

src/latbin/lattice-lmrescore-kaldi-rnnlm.cc

+
+    if (!IsSimpleNnet(rnnlm))
+      KALDI_ERR << "Input RNNLM in " << rnnlm_rxfilename
+                << " is not the type of neural net we were looking for; "


Your Jedi mind tricks won't work on me.

danpovey

A few more small comments on the code

danpovey · 2017-10-12T02:53:42Z

src/rnnlm/rnnlm-compute-state.cc

+                              word_embedding_mat.Row(word_index));
+  if (info_.opts.normalize_probs) {
+    log_prob -= normalization_factor_;
+  }


you should probably add a comment saying that even without explicit normalization, the log-probs will be close to correctly normalized due to the way the model was trained.

danpovey · 2017-10-12T02:54:10Z

src/rnnlm/rnnlm-compute-state.cc

+
+void RnnlmComputeState::AdvanceChunk() {
+  CuMatrix<BaseFloat> input_embeddings(1, info_.word_embedding_mat.NumCols());
+  input_embeddings.RowRange(0, 1).AddMat(1.0, info_.word_embedding_mat.RowRange(previous_word_, 1), kNoTrans);


watch line length.
And since you are using vectors here, better to use input_embeddings.Row(0).AddVec(...), and word_embedding_mat.Row(previous_word_).

danpovey · 2017-10-12T02:55:22Z

src/rnnlm/rnnlm-compute-state.h

+
+#include <vector>
+#include "base/kaldi-common.h"
+#include "gmm/am-diag-gmm.h"


remove unused includes.

danpovey · 2017-10-12T02:56:06Z

src/rnnlm/rnnlm-compute-state.h

+  This class handles the neural net computation; it's mostly accessed
+  via other wrapper classes.
+
+  It accept just input word as features */


No need to mention features-- the interface doesn't really work that way now.

danpovey · 2017-10-12T02:56:35Z

src/rnnlm/rnnlm-compute-state.h

+  /// copy constructor
+  RnnlmComputeState(const RnnlmComputeState &other);
+
+  /// generate another state by passing the next-word


All comments should start with a capital and be properly punctuated (periods at end of sentence) per Google style guide.

danpovey · 2017-10-12T02:57:09Z

src/rnnlm/rnnlm-compute-state.h

+  NnetComputer computer_;
+  int32 previous_word_;
+
+  // this is the log of the sum of the exp'ed values in the output


mention that it is only used if config_.normalize is true (or whatever it's called).

danpovey · 2017-10-12T02:57:54Z

src/rnnlm/rnnlm-compute-state.h

+#include "rnnlm/rnnlm-core-compute.h"
+
+namespace kaldi {
+namespace nnet3 {


I think it would be better to put this in namespace rnnlm-- IIRC, one exists.
You may have to qualify more of its dependencies with 'nnet3::'.

danpovey · 2017-10-12T02:58:14Z

src/rnnlm/rnnlm-compute-state.h

+    opts->Register("normalize-probs", &normalize_probs, "If true, word "
+       "probabilities will be correctly normalized (otherwise the sum-to-one "
+       "normalization is approximate)");
+    opts->Register("bos-symbol", &bos_index, "index in wordlist representing "


capitalize the sentences here.

danpovey · 2017-10-12T03:04:10Z

src/rnnlm/rnnlm-lattice-rescoring.cc

+
+KaldiRnnlmDeterministicFst::~KaldiRnnlmDeterministicFst() {
+  int size = state_to_rnnlm_state_.size();
+  for (int i = 0; i < size; i++) {


no need for the braces. We'd normally either use int32 or size_t here though- just because int32 is the default type for integers and size_t is the actual type of size().

danpovey · 2017-10-15T21:12:02Z

egs/swbd/s5c/local/rnnlm/tuning/run_lstm_f.sh

@@ -0,0 +1,87 @@
+#!/bin/bash
+


please rename this to 1a. I prefer to start from there for new things. There is no need for consistency with your private experiments.

danpovey

A few comments, all cosmetic in one way or another.

danpovey · 2017-11-03T20:57:35Z

egs/swbd/s5c/local/rnnlm/run_rescoring2.sh

+
+  # Lattice rescoring
+  rnnlm/lmrescore_rnnlm_lat_pruned.sh \
+    --cmd "$decode_cmd -l hostname=b*" \


hostname=b* is presumably not needed here.

Actually, on second thoughts I don't think my idea of making this run_rescoring2.sh script into a utility script has much value, because it contains only one script call.
I think what would be better is to put it as a final stage in local/rnnlm/run_lstm.sh, and make the 'decode_dir' an argument to that script (with a default that works as-is for you). Most of my other comments still apply (e.g. about the name of the decode-dir).

Now my previous request to rename run_lstm.sh to train_lstm.sh is no longer relevant.

BTW I think it would be a good idea to rename rnnlm/lmrescore_rnnlm_lat_pruned.sh to rnnlm/rescore_lat_pruned.sh (and make similar name changes for other scripts). 'lm' and 'rnnlm' are already implied by the location, so no need to include them in the script name.

danpovey · 2017-11-03T20:58:45Z

egs/swbd/s5c/local/rnnlm/run_rescoring2.sh

+
+ngram_order=3
+rnndir=exp/rnnlm_lstm_e
+


maybe you should make the 'dir' an option.
If this script becomes more like a utility script, then it will be necessary to give examples from the previous stages (RNNLM-building and acoustic model-building example scripts) to show how to call this. Or at least sufficiently prominently-placed comments. The point is, I want the user to have a clear path towards knowing how to use this script, placed in a natural place where they will find it.

danpovey · 2017-11-03T20:58:59Z

egs/swbd/s5c/local/rnnlm/run_rescoring2.sh

@@ -0,0 +1,27 @@
+#!/bin/bash
+
+ngram_order=3


there needs to be a comment explaining what ngram_order is.

danpovey · 2017-11-03T20:59:17Z

egs/swbd/s5c/local/rnnlm/run_rescoring2.sh

@@ -0,0 +1,27 @@
+#!/bin/bash
+


Rename to run_rescoring.sh

danpovey · 2017-11-03T20:59:40Z

egs/swbd/s5c/local/rnnlm/tuning/run_lstm.sh

@@ -0,0 +1 @@
+run_lstm_1c.sh


this link should be one directory up.
And since these files do training, not decoding, I think it would be more appropriate to call them 'train_lstm.sh' and the like.

danpovey · 2017-11-03T21:49:50Z

egs/swbd/s5c/local/rnnlm/tuning/run_lstm_1d.sh

+if [ $stage -le 0 ]; then
+  mkdir -p $text_dir
+  echo -n >$text_dir/dev.txt
+  # hold out one in every 500 lines as dev data.


This doesn't look right to me.
In this situation, we care about the Switchboard data, so it would be appropriate to hold out some lines of the Switchboard data as dev. If you include Fisher data instead, we can't compare the perplexities with perplexities on Switchboard. In general you can only compare perplexities if the data is the same. You might want to look at other papers reporting Swbd perplexities to see if there is some standard dev dataset that they report on.
Also 1 in 500 lines may not be enough if you include just Swbd data.

danpovey · 2017-11-03T21:53:31Z

egs/swbd/s5c/local/rnnlm/run_rescoring2.sh

+    --weight $weight --max-ngram-order $ngram_order \
+    data/lang_$LM $rnndir \
+    data/${decode_set}_hires ${decode_dir} \
+    ${decode_dir}.kaldirnnlm.lat.${ngram_order}gram.pruned.e


I prefer underscores over periods to separate parts of a directory name. And I think this name is too long and too highly decorated. ${decode_dir} is already quite long. I suggest to have an argument to the script called "decode_dir_suffix", defaulting to just "rnnlm", and call this directory
${decode_dir}_${decode_dir_suffix}.

danpovey · 2017-11-03T22:13:56Z

src/latbin/Makefile

           lattice-lmrescore-pruned

 OBJFILES =

+cuda-compiled.o: ../kaldi.mk


I don't know where this line came from but it shouldn't be here.

danpovey · 2017-11-03T22:17:11Z

src/latbin/lattice-lmrescore-kaldi-rnnlm.cc

+
+    for (; !compact_lattice_reader.Done(); compact_lattice_reader.Next()) {
+      std::string key = compact_lattice_reader.Key();
+      CompactLattice clat = compact_lattice_reader.Value();


Due to changes in how the reader classes are implemented, you can now make 'clat' a reference, and delete the line that says FreeCurrent(). You can do the same in wherever you got this code from, as well.

danpovey · 2017-11-03T22:19:41Z

src/rnnlm/rnnlm-compute-state.cc

+  KALDI_ASSERT(IsSimpleNnet(rnnlm));
+  int32 left_context, right_context;
+  ComputeSimpleNnetContext(rnnlm, &left_context, &right_context);
+  KALDI_ASSERT(0 == left_context);


I think it would be better to check the conditions with a single if, and use KALDI_ERR. That way the check will be active even in non-debug mode (this matters becaue it could happen due to user error, not just code error). The same for the embedding-dim check. Give a nice informative error message there -- it's a likely scripting problem.

danpovey

A few more comments.

danpovey · 2017-11-13T21:01:41Z

egs/swbd/s5c/local/rnnlm/tuning/run_lstm_1a.sh

+. cmd.sh
+. utils/parse_options.sh
+
+text=data/train/text


Be careful with WER numbers on the train_dev subset if you include all the training data here. You might want to use data/train_nodev as the LM training data, and data/train_dev as the dev data.

There is no data/train_nodev or train_dev folder there. I am not sure what you mean here..

danpovey · 2017-11-13T21:02:45Z

egs/swbd/s5c/local/rnnlm/tuning/run_lstm_1a.sh

+
+text=data/train/text
+lexicon=data/local/dict_nosp/lexiconp.txt
+text_dir=data/rnnlm/text_nosp


Our normal practice for Swbd is to include Fisher data as part of the LM training data. Is there are reason you are not doing this here?

data/train/text already includes both swbd and fisher data. It must have been done in previous data-preparation steps.

danpovey · 2017-11-13T21:03:52Z

egs/swbd/s5c/local/rnnlm/tuning/run_lstm_1a.sh

+fi
+
+if [ $stage -le 4 ] && $run_rescore; then
+  echo Perform lattice-rescoring on $ac_model_dir


All echo statements should start with "$0: ". This means that things on the screen always have a clear source.

danpovey · 2017-11-13T21:05:10Z

egs/swbd/s5c/local/rnnlm/tuning/run_lstm_1c.sh

@@ -0,0 +1,111 @@
+#!/bin/bash
+


For each new tuning script I'd like to have a comment saying what it was modified from and comparing the results with that old script, so we have some idea what the performance difference is. Talk to @keli78, maybe she can help write a script to automate this comparison of directories. What would be really nice is if you could have a script in local/rnnlm/ that prints out the train and dev perplexities as well as the decoding results, but I don't know how feasible that would be.

danpovey · 2017-11-13T21:06:17Z

egs/swbd/s5c/local/rnnlm/tuning/run_lstm_1e.sh

@@ -0,0 +1,114 @@
+#!/bin/bash
+
+# Copyright 2012  Johns Hopkins University (author: Daniel Povey)


There may be more tuning scripts there than we really need. My practice is not to check in most of my tuning experiments but mostly limit it to the successful ones. It's hard to judge how useful these are, because there aren't comments saying what you changed and how it affected the results.

danpovey · 2017-11-13T21:08:35Z

scripts/rnnlm/lmrescore_rnnlm_lat.sh

@@ -0,0 +1,109 @@
+#!/bin/bash
+


I don't like the 'rnnlm' in the name here because it's redundant-- it's already in a directory called 'rnnlm'. You could call it 'lmrescore_lat.sh', but IMO the name 'lmrescore.sh' would be more ideal, and you could then have the n-best one be called 'lmrescore_nbest.sh', if needed.

danpovey · 2017-11-13T21:09:19Z

scripts/rnnlm/lmrescore_rnnlm_lat.sh

+# Begin configuration section.
+cmd=run.pl
+skip_scoring=false
+max_ngram_order=4


Clarify via a comment that this is for the n-gram approximation in lattice rescoring, and if feasible, refer to some kind of paper that explains this concept.

danpovey · 2017-11-13T21:09:29Z

scripts/rnnlm/lmrescore_rnnlm_lat.sh

+cmd=run.pl
+skip_scoring=false
+max_ngram_order=4
+N=10


I assume this N variable is no longer needed.

danpovey · 2017-11-13T21:10:03Z

scripts/rnnlm/lmrescore_rnnlm_lat.sh

+max_ngram_order=4
+N=10
+weight=1.0  # Interpolation weight for RNNLM.
+normalize=false


Clarify via a comment what this means, and again a reference to a paper might be nice. You can send me the papers to upload to my website if you want, this means we can use a URL here.

danpovey · 2017-11-13T22:41:14Z

src/latbin/lattice-lmrescore-kaldi-rnnlm.cc

+      KALDI_ERR << "You must set --bos-symbol and --eos-symbol options";
+    }
+
+    std::string lats_rspecifier, word_embedding_rxfilename,


I would prefer defining these as you declare them.

danpovey · 2017-11-16T21:52:25Z

I guess you didn't run the acoustic-model training aspects of this? Or something is confusing me here. Because you are in the directory swbd/s5c. The acoustic model training data (in data/train) is Swbd-only. But later on the run.sh separates it into train_dev (which we'll use as a dev-set for decoding) and train_nodev, from which other subsets are created, which we'll train the acoustic models on. So I'm saying maybe you should train the LM on train_nodev (so that the decoding on train_dev is valid), and also include Fisher, which is certainly not in data/train. You'll have to look at the LM training scripts there to see where the Fisher data is.

…

On Thu, Nov 16, 2017 at 4:41 PM, hainan-xv ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In egs/swbd/s5c/local/rnnlm/tuning/run_lstm_1a.sh <#1906 (comment)>: > + +# variables for lattice rescoring +run_rescore=false +ac_model_dir=exp/chain/tdnn_lstm_1e_sp +decode_dir_suffix=rnnlm +ngram_order=4 # approximate the lattice-rescoring by limiting the max-ngram-order + # if it's set, it merges histories in the lattice if they share + # the same ngram history and this prevents the lattice from + # exploding exponentially + +. cmd.sh +. utils/parse_options.sh + +text=data/train/text +lexicon=data/local/dict_nosp/lexiconp.txt +text_dir=data/rnnlm/text_nosp data/train/text already includes both swbd and fisher data. It must have been done in previous data-preparation steps. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1906 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVuxw_1fPMQOIZDCyqPaTUN7LIDN-sks5s3KwdgaJpZM4Ph_FH> .

hainan-xv · 2017-11-16T21:59:27Z

Oh my models were copied from other people. It might've been in s5 originally. I found a working s5c recipe from @freewym. I will update the scripts and test on the new files. On Thu, Nov 16, 2017 at 4:52 PM, Daniel Povey <notifications@github.com> wrote:

…

I guess you didn't run the acoustic-model training aspects of this? Or something is confusing me here. Because you are in the directory swbd/s5c. The acoustic model training data (in data/train) is Swbd-only. But later on the run.sh separates it into train_dev (which we'll use as a dev-set for decoding) and train_nodev, from which other subsets are created, which we'll train the acoustic models on. So I'm saying maybe you should train the LM on train_nodev (so that the decoding on train_dev is valid), and also include Fisher, which is certainly not in data/train. You'll have to look at the LM training scripts there to see where the Fisher data is. On Thu, Nov 16, 2017 at 4:41 PM, hainan-xv ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In egs/swbd/s5c/local/rnnlm/tuning/run_lstm_1a.sh > <#1906 (comment)>: > > > + > +# variables for lattice rescoring > +run_rescore=false > +ac_model_dir=exp/chain/tdnn_lstm_1e_sp > +decode_dir_suffix=rnnlm > +ngram_order=4 # approximate the lattice-rescoring by limiting the max-ngram-order > + # if it's set, it merges histories in the lattice if they share > + # the same ngram history and this prevents the lattice from > + # exploding exponentially > + > +. cmd.sh > +. utils/parse_options.sh > + > +text=data/train/text > +lexicon=data/local/dict_nosp/lexiconp.txt > +text_dir=data/rnnlm/text_nosp > > data/train/text already includes both swbd and fisher data. It must have > been done in previous data-preparation steps. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#1906 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ADJVuxw_ 1fPMQOIZDCyqPaTUN7LIDN-sks5s3KwdgaJpZM4Ph_FH> > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1906 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFMCDt0m7DwJ6sH7ZuGe0QIKdcvUr3FDks5s3K61gaJpZM4Ph_FH> .

-- - Hainan

…ts for reference

francoishernandez · 2017-11-20T18:05:28Z

Hi there,
I'm trying to build this to compare with our TF setup but it keeps failing. (Even on a clean clone.)
I tried both Dan's main rnnlm branch and Hainan's rnnlm-rescoring one, but I keep getting the following error:

sampling-lm-estimate.cc:438:46: error: no matching function for call to ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::insert(std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::const_iterator&, kaldi::rnnlm::SamplingLmEstimator::Count&)’
             prev_state->counts.insert(iter, c);
                                              ^

I know it's still work in progress but as you already published some results I wanted to try.
Thanks in advance!
F

Here is the rest of the logs:

sampling-lm-estimate.cc:438:46: note: candidates are:
In file included from /usr/include/c++/4.8/vector:69:0,
                 from /home/moses/kaldi/tools/openfst/include/fst/compat.h:24,
                 from /home/moses/kaldi/tools/openfst/include/fst/fst.h:21,
                 from /home/moses/kaldi/tools/openfst/include/fst/expanded-fst.h:16,
                 from /home/moses/kaldi/tools/openfst/include/fst/fstlib.h:28,
                 from ../rnnlm/sampling-lm-estimate.h:23,
                 from sampling-lm-estimate.cc:21:
/usr/include/c++/4.8/bits/vector.tcc:107:5: note: std::vector<_Tp, _Alloc>::iterator std::vector<_Tp, _Alloc>::insert(std::vector<_Tp, _Alloc>::iterator, const value_type&) [with _Tp = kaldi::rnnlm::SamplingLmEstimator::Count; _Alloc = std::allocator<kaldi::rnnlm::SamplingLmEstimator::Count>; std::vector<_Tp, _Alloc>::iterator = __gnu_cxx::__normal_iterator<kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >; typename std::_Vector_base<_Tp, _Alloc>::pointer = kaldi::rnnlm::SamplingLmEstimator::Count*; std::vector<_Tp, _Alloc>::value_type = kaldi::rnnlm::SamplingLmEstimator::Count]
     vector<_Tp, _Alloc>::
     ^
/usr/include/c++/4.8/bits/vector.tcc:107:5: note:   no known conversion for argument 1 from ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::const_iterator {aka __gnu_cxx::__normal_iterator<const kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >}’ to ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::iterator {aka __gnu_cxx::__normal_iterator<kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >}’
In file included from /usr/include/c++/4.8/vector:64:0,
                 from /home/moses/kaldi/tools/openfst/include/fst/compat.h:24,
                 from /home/moses/kaldi/tools/openfst/include/fst/fst.h:21,
                 from /home/moses/kaldi/tools/openfst/include/fst/expanded-fst.h:16,
                 from /home/moses/kaldi/tools/openfst/include/fst/fstlib.h:28,
                 from ../rnnlm/sampling-lm-estimate.h:23,
                 from sampling-lm-estimate.cc:21:
/usr/include/c++/4.8/bits/stl_vector.h:988:7: note: std::vector<_Tp, _Alloc>::iterator std::vector<_Tp, _Alloc>::insert(std::vector<_Tp, _Alloc>::iterator, std::vector<_Tp, _Alloc>::value_type&&) [with _Tp = kaldi::rnnlm::SamplingLmEstimator::Count; _Alloc = std::allocator<kaldi::rnnlm::SamplingLmEstimator::Count>; std::vector<_Tp, _Alloc>::iterator = __gnu_cxx::__normal_iterator<kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >; typename std::_Vector_base<_Tp, _Alloc>::pointer = kaldi::rnnlm::SamplingLmEstimator::Count*; std::vector<_Tp, _Alloc>::value_type = kaldi::rnnlm::SamplingLmEstimator::Count]
       insert(iterator __position, value_type&& __x)
       ^
/usr/include/c++/4.8/bits/stl_vector.h:988:7: note:   no known conversion for argument 1 from ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::const_iterator {aka __gnu_cxx::__normal_iterator<const kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >}’ to ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::iterator {aka __gnu_cxx::__normal_iterator<kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >}’
/usr/include/c++/4.8/bits/stl_vector.h:1005:7: note: void std::vector<_Tp, _Alloc>::insert(std::vector<_Tp, _Alloc>::iterator, std::initializer_list<_Tp>) [with _Tp = kaldi::rnnlm::SamplingLmEstimator::Count; _Alloc = std::allocator<kaldi::rnnlm::SamplingLmEstimator::Count>; std::vector<_Tp, _Alloc>::iterator = __gnu_cxx::__normal_iterator<kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >; typename std::_Vector_base<_Tp, _Alloc>::pointer = kaldi::rnnlm::SamplingLmEstimator::Count*]
       insert(iterator __position, initializer_list<value_type> __l)
       ^
/usr/include/c++/4.8/bits/stl_vector.h:1005:7: note:   no known conversion for argument 1 from ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::const_iterator {aka __gnu_cxx::__normal_iterator<const kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >}’ to ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::iterator {aka __gnu_cxx::__normal_iterator<kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >}’
/usr/include/c++/4.8/bits/stl_vector.h:1023:7: note: void std::vector<_Tp, _Alloc>::insert(std::vector<_Tp, _Alloc>::iterator, std::vector<_Tp, _Alloc>::size_type, const value_type&) [with _Tp = kaldi::rnnlm::SamplingLmEstimator::Count; _Alloc = std::allocator<kaldi::rnnlm::SamplingLmEstimator::Count>; std::vector<_Tp, _Alloc>::iterator = __gnu_cxx::__normal_iterator<kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >; typename std::_Vector_base<_Tp, _Alloc>::pointer = kaldi::rnnlm::SamplingLmEstimator::Count*; std::vector<_Tp, _Alloc>::size_type = long unsigned int; std::vector<_Tp, _Alloc>::value_type = kaldi::rnnlm::SamplingLmEstimator::Count]
       insert(iterator __position, size_type __n, const value_type& __x)
       ^
/usr/include/c++/4.8/bits/stl_vector.h:1023:7: note:   candidate expects 3 arguments, 2 provided
/usr/include/c++/4.8/bits/stl_vector.h:1044:9: note: template<class _InputIterator, class> void std::vector<_Tp, _Alloc>::insert(std::vector<_Tp, _Alloc>::iterator, _InputIterator, _InputIterator) [with _InputIterator = _InputIterator; <template-parameter-2-2> = <template-parameter-1-2>; _Tp = kaldi::rnnlm::SamplingLmEstimator::Count; _Alloc = std::allocator<kaldi::rnnlm::SamplingLmEstimator::Count>]
         insert(iterator __position, _InputIterator __first,
         ^
/usr/include/c++/4.8/bits/stl_vector.h:1044:9: note:   template argument deduction/substitution failed:
sampling-lm-estimate.cc:438:46: note:   cannot convert ‘iter’ (type ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::const_iterator {aka __gnu_cxx::__normal_iterator<const kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >}’) to type ‘std::vector<kaldi::rnnlm::SamplingLmEstimator::Count>::iterator {aka __gnu_cxx::__normal_iterator<kaldi::rnnlm::SamplingLmEstimator::Count*, std::vector<kaldi::rnnlm::SamplingLmEstimator::Count> >}’
             prev_state->counts.insert(iter, c);
                                              ^
g++ -std=c++11 -I.. -I/home/moses/kaldi/tools/openfst/include  -Wall -Wno-sign-compare -Wno-unused-local-typedefs -Wno-deprecated-declarations -Winit-self -DKALDI_DOUBLEPRECISION=0 -DHAVE_EXECINFO_H=1 -DHAVE_CXXABI_H -DHAVE_ATLAS -I/home/moses/kaldi/tools/ATLAS/include -msse -msse2 -pthread -g  -fPIC -DHAVE_CUDA -I/usr/local/cuda/include   -c -o rnnlm-lattice-rescoring.o rnnlm-lattice-rescoring.cc
make[2]: *** [sampling-lm-estimate.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/home/moses/kaldi/src/rnnlm'
make[1]: *** [rnnlm] Error 2
make[1]: *** Waiting for unfinished jobs....
ar -cru kaldi-online2.a online-gmm-decodable.o online-feature-pipeline.o online-ivector-feature.o online-nnet2-feature-pipeline.o online-gmm-decoding.o online-timing.o online-endpoint.o onlinebin-util.o online-speex-wrapper.o online-nnet2-decoding.o online-nnet2-decoding-threaded.o online-nnet3-decoding.o
ranlib kaldi-online2.a
g++ -shared -o libkaldi-online2.so -Wl,--no-undefined -Wl,--as-needed  -Wl,-soname=libkaldi-online2.so,--whole-archive kaldi-online2.a -Wl,--no-whole-archive  -Wl,-rpath=/home/moses/kaldi/tools/openfst/lib -rdynamic -Wl,-rpath=/home/moses/kaldi/src/lib  ../ivector/libkaldi-ivector.so  ../nnet3/libkaldi-nnet3.so  ../chain/libkaldi-chain.so  ../nnet2/libkaldi-nnet2.so  ../cudamatrix/libkaldi-cudamatrix.so  ../decoder/libkaldi-decoder.so  ../lat/libkaldi-lat.so  ../hmm/libkaldi-hmm.so  ../feat/libkaldi-feat.so  ../transform/libkaldi-transform.so  ../gmm/libkaldi-gmm.so  ../tree/libkaldi-tree.so  ../util/libkaldi-util.so  ../matrix/libkaldi-matrix.so  ../base/libkaldi-base.so /home/moses/kaldi/tools/openfst/lib/libfst.so /usr/lib/libatlas.so.3 /usr/lib/libf77blas.so.3 /usr/lib/libcblas.so.3 /usr/lib/liblapack_atlas.so.3 -lm -lpthread -ldl
ln -sf /home/moses/kaldi/src/online2/libkaldi-online2.so /home/moses/kaldi/src/lib/libkaldi-online2.so
make[2]: Leaving directory `/home/moses/kaldi/src/online2'
make[1]: Leaving directory `/home/moses/kaldi/src'
make: *** [all] Error 2

danpovey · 2017-11-20T18:15:57Z

Your compiler seems to be wanting a non-const iterator.
According to the documentation here
http://en.cppreference.com/w/cpp/container/vector/insert
since C++11 a const iterator should be fine, so I guess your compiler has a non-conformant stl library, but anyway I'm pushing a workaround for the problem.

danpovey · 2017-11-23T19:14:33Z

I'm not doing a careful review of this again but I'll merge this as the rnnlm branch isn't so useful without it.

hainan-xv added 7 commits September 18, 2017 16:32

draft

0d839b0

lattice-rescoring draft finished

699c956

lattice-rescoring runnable but buggy

ef09b62

making a PR

390a1bb

small changes

dc49709

include lmrescore_rnn_lat.sh

8a33e77

Merge branch 'rnnlm' into rnnlm-rescoring

5965b87

danpovey reviewed Sep 24, 2017

View reviewed changes

hainan-xv added 3 commits September 25, 2017 16:03

some aesthetic changes; not final yet

483450d

Merge branch 'master' into rnnlm-rescoring

00912f7

cached version of lattice rescoring; buggy it seems

b1167a2

danpovey reviewed Sep 27, 2017

View reviewed changes

hainan-xv added 4 commits September 27, 2017 15:43

purely aesthetic changes

a52da29

re-written some of the classes

3bdaa4d

very small changes

2b08335

fix a typo

7cf4af8

danpovey reviewed Sep 30, 2017

View reviewed changes

make RNNLM share the same FST wordlist

8f35242

fix small issue when running lattice-rescoring with normalize-probs o…

705ecc8

…ption

danpovey reviewed Oct 6, 2017

View reviewed changes

minor changes

d19ecc1

danpovey reviewed Oct 11, 2017

View reviewed changes

danpovey reviewed Oct 12, 2017

View reviewed changes

hainan-xv added 3 commits October 14, 2017 16:54

fix small stylistic issues in code

232ef04

fix wrong variable used in scripts/rnnlm/lmrescore_rnnlm_lat.sh

bd9936b

add rnnlm softlink in swbd/s5c

9cc7ba1

danpovey reviewed Oct 15, 2017

View reviewed changes

hainan-xv added 2 commits October 30, 2017 15:47

small style changes

267177f

merge with latest upstream

87f2f6c

danpovey reviewed Nov 3, 2017

View reviewed changes

hainan-xv added 3 commits November 7, 2017 18:15

move rescoring into rnnlm training scripts

c9bf5e0

move rescoring into rnnlm training scripts

091d4d5

fix small issues mentioned by @danoneata

a192ada

danpovey reviewed Nov 13, 2017

View reviewed changes

hainan-xv added 2 commits November 19, 2017 23:55

change SWBD script to accommodate s5_c; add paper link to RNNLM scrip…

697f219

…ts for reference

fix conflicts

acb5211

danpovey merged commit 652050a into kaldi-asr:rnnlm Nov 23, 2017

hainan-xv deleted the rnnlm-rescoring branch December 21, 2017 01:55


		inline int32 NnetOutputDim() const { return info_.nnet_output_dim; }

		// Gets the nnet's output for a particular frame, with 0 <= frame < NumFrames().

		// int32 num_words = info_.word_embedding_mat.NumRows();


		CuMatrix<BaseFloat> current_nnet_output_gpu;

		@@ -0,0 +1,114 @@
		#!/bin/bash

		# Copyright 2012 Johns Hopkins University (author: Daniel Povey)

		fi

		touch $dir/unk.probs

nnet3-rnnlm lattice rescoring draft #1906

nnet3-rnnlm lattice rescoring draft #1906

Conversation

hainan-xv commented Sep 24, 2017

Choose a reason for hiding this comment

danpovey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

hainan-xv commented Sep 27, 2017 via email

danpovey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey commented Sep 27, 2017 via email

danpovey commented Sep 27, 2017 via email

Choose a reason for hiding this comment

hainan-xv commented Sep 30, 2017 via email

danpovey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey Sep 27, 2017 •

edited

Loading

danpovey Sep 27, 2017 •

edited

Loading