change DropoutComponent into by-frame mode #1324

GaofengCheng · 2017-01-09T01:26:36Z

i.e. dropout matrix will be setted like
[[1,1,1,1],[0,0,0,0],[0,0,0,0],[1,1,1,1],[0,0,0,0]]

i.e. dropout matrix will be setted like [[1,1,1,1],[0,0,0,0],[0,0,0,0],[1,1,1,1],[0,0,0,0]]

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

src/nnet3/nnet-simple-component.cc

- out->ApplyHeaviside(); // apply the function (x>0?1:0). Now, a proportion "dropout" will
- // be zero and (1 - dropout) will be 1.0.
-
+ // now, a proportion "dropout" will be <0.0


This wouldn't be compatible with regular uses of dropout. You should add an option to the component.
And it's wasteful to do get a whole random matrix and only use one column; you should use a submatrix consisting of one column and randomly set just that.
Try again and I'll comment; this may take a few tries.

@danpovey I add the option to choose by frame or the existing dropout in c++ level in this PR, after Vimal's code is merged, then adding the scripts level drop mode choice, will this be OK?

There doesn't necessarily have to be an option in the scripts, it can be hardcoded; and anyway we'll consider that later; but the component should support both options. e.g. adding "dropout-per-frame=true" in the config file to the component, will set the per-frame mode.

danpovey · 2017-01-10T20:57:00Z

src/nnet3/nnet-simple-component.cc

+ // apply the function (x>0?1:0). Now, a proportion "dropout" will
+ // be zero and (1 - dropout) will be 1.0.
+ out->ApplyHeaviside();
+ CuVector<BaseFloat> *random_drop_vector = new CuVector<BaseFloat>(in.NumRows(), kSetZero);


this is not how you create a subvector, and it's a memory leak. Please read the following carefully:
http://kaldi-asr.org/doc/matrix.html

OK, thx for information!

danpovey · 2017-01-13T06:51:55Z

src/nnet3/nnet-simple-component.cc

- KALDI_ERR << "Invalid initializer for layer of type "
- << Type() << ": \"" << cfl->WholeLine() << "\"";
- Init(dim, dropout_proportion);
+ bool ok2 = cfl->GetValue("dropout-per-frame", &dropout_per_frame);


This complicated logic for checking dropout-per-frame is unnecessary. Just declare it as false
bool dropout_per_frame = false;
and don't check the return status of cfl->GetValue(), since it's optional.
No checking is needed since the code cannot set it to values other than true or false.

danpovey · 2017-01-13T06:52:07Z

src/nnet3/nnet-simple-component.cc

 stream << Type() << ", dim=" << dim_
- << ", dropout-proportion=" << dropout_proportion_;
+ << ", dropout-proportion=" << dropout_proportion_
+ << ", dropout-per-frame=" <<dropout_per_frame_;


space after <<

danpovey · 2017-01-13T06:52:42Z

src/nnet3/nnet-simple-component.cc

- // be zero and (1 - dropout) will be 1.0.
-
- out->MulElements(in);
+ if(!dropout_per_frame_)


if (!dropout_per_frame_) {
...
use cpplint.py to check the c++ style, it's in misc/ I think.

danpovey · 2017-01-13T07:03:04Z

src/nnet3/nnet-simple-component.cc

+ // This const_cast is only safe assuming you don't attempt
+ // to use multi-threaded code with the GPU.
+ CuSubMatrix<BaseFloat> out_col_submatrix = out->ColRange(0, 1);
+ const_cast<CuRand<BaseFloat>&>(random_generator_).RandUniform(&out_col_submatrix); 


Since you are having to copy the matrix to a vector, and having to allocate a vector anyway, it would be better to set the vector to random directly. But it will be more convenient to allocate it as a matrix, like:
CuMatrix tmp(1, out->NumCols(), kUndefined);
const_cast<...>(random_generator_).RandUniform(&tmp);
..
out->AddVecToCols(1.0, tmp.Row(0), 0.0);

@danpovey CuMatrix tmp(1, out->NumCols(), kUndefined); ? or We should use CuMatrix tmp(out->NumRow(), 1, kUndefined); We want to copy the same col to the whole matrix(so matrix will be the same in row dim), the tmp should be row_num1, not 1col_num. Is this right?

danpovey · 2017-01-13T07:03:44Z

src/nnet3/nnet-simple-component.cc

+ }
+ KALDI_ASSERT(token == "<Dim>");
+ ReadBasicType(is, binary, &dim_); // read dimension.
+ ReadToken(is, binary, &token);


You can just do
ExpectToken(is, binary, "")
instead of reading the token adn then checking it.

@danpovey maybe we should decide whether to be back compatible to previous model. Previous DropoutComponent do not have per-frame opts. This checking aims to avoid crash. ExpectToken(is, binary, "<DropoutPerFrame>") on elder model will go wrong.

danpovey · 2017-01-13T07:04:16Z

src/nnet3/nnet-simple-component.cc

+ if(token == "<DropoutPerFrame>"){
+ ReadBasicType(is, binary, &dropout_per_frame_); // read dropout mode
+ ReadToken(is, binary, &token);
+ KALDI_ASSERT(token == "</DropoutComponent>");


you can move the duplicated assert past the if-statement.
and remember the spaces:
if (...) {

danpovey · 2017-01-13T07:05:03Z

src/nnet3/nnet-simple-component.h

+ DropoutComponent(int32 dim, BaseFloat dropout = 0.0,
+ bool dropout_per_frame = true) {
+ Init(dim, dropout, dropout_per_frame);
+ }


wrong indentation levels.

danpovey · 2017-01-13T07:05:26Z

src/nnet3/nnet-simple-component.h

+ }

- DropoutComponent(): dim_(0), dropout_proportion_(0.0) { }
+ DropoutComponent(): dim_(0), dropout_proportion_(0.0), dropout_per_frame_(true) { }


run cpplint.py and check you don't go past 80 chars.

danpovey · 2017-01-13T07:07:12Z

src/nnet3bin/nnet3-copy.cc

- if (dropout > 0)
- SetDropoutProportion(dropout, &nnet);
+ if (dropout >= 0)
+ KALDI_ERR << "--dropout option is deprecated. "


[this for Vimal, but]... "deprecated" means still supported but not recommended. Since this will crash, deprecated is not the right term. I would suggest to just remove the set-dropout-proportion option and related code from the program. If no scripts are using it, this is OK.

…pout_per_frame

danpovey · 2017-01-18T04:29:54Z

No, tmp(1, out->NumCols(), kUndefined); was correct. Then you can do tmp.Row(0) to get the 1st row, and call CopyColsFromVec() to copy that row to each column of the matrix.

…

On Tue, Jan 17, 2017 at 9:50 PM, Gaofeng Cheng ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/nnet3/nnet-simple-component.cc <#1324>: > @@ -150,11 +186,26 @@ void DropoutComponent::Backprop(const std::string &debug_info, void DropoutComponent::Read(std::istream &is, bool binary) { - ExpectOneOrTwoTokens(is, binary, "<DropoutComponent>", "<Dim>"); - ReadBasicType(is, binary, &dim_); - ExpectToken(is, binary, "<DropoutProportion>"); - ReadBasicType(is, binary, &dropout_proportion_); - ExpectToken(is, binary, "</DropoutComponent>"); + //back-compatibility code. + std::string token; + ReadToken(is, binary, &token); + if(token == "<DropoutComponent>"){ + ReadToken(is, binary, &token); + } + KALDI_ASSERT(token == "<Dim>"); + ReadBasicType(is, binary, &dim_); // read dimension. + ReadToken(is, binary, &token); @danpovey <https://github.com/danpovey> maybe we should decide whether to be back compatible to previous model. Previous DropoutComponent do not have per-frame opts. This checking aims to avoid crash. ExpectToken(is, binary, "<DropoutPerFrame>") on elder model will go wrong. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1324>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVuzYgk6woI81tKHJvkNccaxshecsxks5rTX3xgaJpZM4Ld3t8> .

danpovey · 2017-01-18T04:30:44Z

It definitely needs to be back compatible to the old format. Except that code seems to have a bug in it if(token == "<DropoutComponent>"){ should be if(token == "<DropoutPerFrame>"){

…

On Tue, Jan 17, 2017 at 9:50 PM, Gaofeng Cheng ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/nnet3/nnet-simple-component.cc <#1324>: > @@ -150,11 +186,26 @@ void DropoutComponent::Backprop(const std::string &debug_info, void DropoutComponent::Read(std::istream &is, bool binary) { - ExpectOneOrTwoTokens(is, binary, "<DropoutComponent>", "<Dim>"); - ReadBasicType(is, binary, &dim_); - ExpectToken(is, binary, "<DropoutProportion>"); - ReadBasicType(is, binary, &dropout_proportion_); - ExpectToken(is, binary, "</DropoutComponent>"); + //back-compatibility code. + std::string token; + ReadToken(is, binary, &token); + if(token == "<DropoutComponent>"){ + ReadToken(is, binary, &token); + } + KALDI_ASSERT(token == "<Dim>"); + ReadBasicType(is, binary, &dim_); // read dimension. + ReadToken(is, binary, &token); @danpovey <https://github.com/danpovey> maybe we should decide whether to be back compatible to previous model. Previous DropoutComponent do not have per-frame opts. This checking aims to avoid crash. ExpectToken(is, binary, "<DropoutPerFrame>") on elder model will go wrong. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1324>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVuzYgk6woI81tKHJvkNccaxshecsxks5rTX3xgaJpZM4Ld3t8> .

…pout_per_frame

delete complicated logic for checking dropout-per-frame just set true in hard code

GaofengCheng · 2017-01-20T14:40:59Z

src/nnet3/nnet-simple-component.cc

 out->SetZero();
- out->AddVecToCols(1.0 , *random_drop_vector, 1.0);
+ out->AddVecToCols(1.0, tmp.Row(0), 0.0);
 out->MulElements(in);


Change from my previous method (random a whole matrix and select one col) into this one, and a test on AMI SDM is done, they are equivalent on WER:
previous way:

new way:

danpovey · 2017-01-21T19:09:36Z

src/nnet3/nnet-simple-component.h

+ }

- DropoutComponent(): dim_(0), dropout_proportion_(0.0) { }
+ DropoutComponent(): dim_(0), dropout_proportion_(0.0), dropout_per_frame_(true) { }


dropout_per_frame should default to false (default to "normal" dropout).

danpovey · 2017-01-21T19:10:06Z

src/nnet3/nnet-simple-component.cc

 void DropoutComponent::InitFromConfig(ConfigLine *cfl) {
 int32 dim = 0;
 BaseFloat dropout_proportion = 0.0;
+ bool dropout_per_frame = true;


this should default to false.

danpovey · 2017-01-21T19:11:29Z

src/nnet3/nnet-simple-component.cc

 bool ok = cfl->GetValue("dim", &dim) &&
 cfl->GetValue("dropout-proportion", &dropout_proportion);
+ // for this stage, dropout is hard coded in by-frame mode
 if (!ok || cfl->HasUnusedValues() || dim <= 0 ||


where is the "dropout-per-frame" configuration value being read?
You should be doing

cfl->GetValue("dropout-per-frame", &dropout_per_frame);

[don't check the return value of GetValue().]

danpovey · 2017-01-21T19:25:01Z

src/nnet3/nnet-simple-component.cc

+ } else {
+ // randomize the dropout matrix by row,
+ // i.e. [[1,1,1,1],[0,0,0,0],[0,0,0,0],[1,1,1,1],[0,0,0,0]]
+ CuMatrix<BaseFloat> tmp(1, out->NumRows(), kUndefined);


You should now be able to change this code to:
(a) use a vector instead of a matrix for 'tmp', and
(b) use CopyVecToCols() instead of AddVecToCols(), which avoids the need to call SetZero().

In about 20 minutes I'll push @kangshiyin's changes regarding implementing CopyVecToCols() to the master branch; and @vesis84 has already implemented RandUniform for vectors

@danpovey CuVector does not support Heaviside(), cumatrix remianed, but AddVecToCol is replaced with CopyColsFromVec, out->SetZero() is skipped

@danpovey Rerun with this code, results OK

danpovey · 2017-01-21T19:25:55Z

src/nnet3/nnet-simple-component.h


- DropoutComponent(int32 dim, BaseFloat dropout = 0.0) { Init(dim, dropout); }
+ DropoutComponent(int32 dim, BaseFloat dropout = 0.0,
+ bool dropout_per_frame = true) {


the Init and the close-brace are at the wrong indentation level-- much too high. And make sure you are writing spaces and not tabs.

danpovey · 2017-01-21T19:26:25Z

src/nnet3/nnet-simple-component.cc

 void DropoutComponent::InitFromConfig(ConfigLine *cfl) {
 int32 dim = 0;
 BaseFloat dropout_proportion = 0.0;
+ bool dropout_per_frame = true;


you didn't address this comment about 80 chars and cpplint.py

danpovey · 2017-01-21T19:28:49Z

src/nnet3/nnet-utils.cc

+ name_pattern.c_str()) &&
+ (component =
+ dynamic_cast<DropoutComponent*>(nnet->GetComponent(c)))) {
+ component->SetDropoutProportion(proportion);


You should do:

if (component != NULL) { component->SetDropoutProportion(proportion); num_dropout_proportions_set++; }

That way you can call this without specifying the name (your code would have segfaulted).
And please rename 'component' to 'dropout_component'

danpovey · 2017-01-21T19:29:11Z

src/nnet3/nnet-utils.cc

+ }
+ }
+ KALDI_LOG << "Set dropout proportions for "
+ << num_dropout_proportions_set << " nodes.";


please change 'nodes' to 'components'.

danpovey · 2017-01-21T19:31:31Z

src/nnet3bin/nnet3-copy.cc

- 
- if (dropout > 0)
+
+ if (dropout >= 0)


I believe we're removing these dropout options from the scripts.
[however, for this code to work as intended, the default value of 'dropout' should be -1, not zero].
Anyway, we should remove all related options and code and rely on the --edits and --edits-config options.

…pout_per_frame # Conflicts: # src/nnet3bin/nnet3-copy.cc

Cuvector is not used for not supporting Heaviside; dropmode is set in normal mode;

danpovey · 2017-01-25T04:27:44Z

src/nnet3/nnet-simple-component.cc

 stream << Type() << ", dim=" << dim_
- << ", dropout-proportion=" << dropout_proportion_;
+ << ", dropout-proportion=" << dropout_proportion_
+ << ", dropout-per-frame=" << dropout_per_frame_;


you can't do << to print out a bool, it will print 0 or 1.

@danpovey fixed

danpovey · 2017-01-25T04:29:22Z

src/nnet3/nnet-simple-component.cc

- ExpectToken(is, binary, "<DropoutProportion>");
- ReadBasicType(is, binary, &dropout_proportion_);
- ExpectToken(is, binary, "</DropoutComponent>");
+ // back-compatibility code.


remove this comment, it's not accurate (1st token may be missing due to how components are read in, not back compatibility).

change DropoutComponent into by-frame mode

e4621f3

i.e. dropout matrix will be setted like [[1,1,1,1],[0,0,0,0],[0,0,0,0],[1,1,1,1],[0,0,0,0]]

danpovey reviewed Jan 9, 2017

View reviewed changes

danpovey reviewed Jan 10, 2017

View reviewed changes

back-compatible and sub_matrix

600a285

danpovey reviewed Jan 13, 2017

View reviewed changes

Merge remote-tracking branch 'refs/remotes/kaldi-asr/master' into dro…

5a1fd88

…pout_per_frame

GaofengCheng added 3 commits January 20, 2017 21:27

Merge remote-tracking branch 'refs/remotes/kaldi-asr/master' into dro…

2749376

…pout_per_frame

Fix frame dropout code and test

127cf8d

fix Dropout init from config

fe2b9b2

delete complicated logic for checking dropout-per-frame just set true in hard code

GaofengCheng commented Jan 20, 2017

View reviewed changes

danpovey reviewed Jan 21, 2017

View reviewed changes

GaofengCheng mentioned this pull request Jan 23, 2017

Scripts place Diff between fast and normal (B)LSTM #1362

Closed

GaofengCheng added 3 commits January 23, 2017 16:49

Merge remote-tracking branch 'refs/remotes/kaldi-asr/master' into dro…

8a5446d

…pout_per_frame # Conflicts: # src/nnet3bin/nnet3-copy.cc

using CopyColsFromVec instead of AddVecToCols

7b6b517

Cuvector is not used for not supporting Heaviside; dropmode is set in normal mode;

bugfix

ec25c1b

GaofengCheng mentioned this pull request Jan 24, 2017

About By-frame Dropout scripts Level PR #1372

Merged

danpovey reviewed Jan 25, 2017

View reviewed changes

fix '<<' bool

8fc470f

danpovey merged commit 9208165 into kaldi-asr:master Jan 25, 2017

change DropoutComponent into by-frame mode #1324

change DropoutComponent into by-frame mode #1324

Uh oh!

Conversation

GaofengCheng commented Jan 9, 2017

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

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 commented Jan 18, 2017 via email

danpovey commented Jan 18, 2017 via email

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

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

Labels

2 participants