fix embedding training bug for RNNLM without letter features

[hainan-xv]

Addressing issue #2569

[danpovey]

Can you explain what the difference is, or why the old code was wrong?

[hainan-xv]

The old code does

a subset of rows of embedding_mat_ += derivs * l2_factor

But it should be

derivs += a subset of rows of embedding_mat * l2_factor

Later derivs will be added to embeeding_mat with learning rate (and possibly momentum) which makes it different.

[danpovey]

Is it possible to just use AddRows() then? I feel like there may be an unnecessary temporary.

…

On Tue, Jul 31, 2018 at 3:16 PM, hainan-xv ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/rnnlm/rnnlm-embedding-training.cc <#2586 (comment)>: > @@ -175,7 +175,10 @@ void RnnlmEmbeddingTrainer::Train( if (config_.l2_regularize > 0.0) { BaseFloat l2_term = -2 * config_.l2_regularize; if (l2_term != 0.0) { - embedding_deriv->AddToRows(l2_term, active_words, embedding_mat_); + CuMatrix<BaseFloat> l2_matrix(embedding_deriv->NumRows(), + embedding_deriv->NumCols(), kUndefined); The old code does a subset of rows of embedding_mat_ += derivs * l2_factor But it should be derivs += a subset of rows of embedding_mat * l2_factor Later derivs will be added to embeeding_mat with learning rate (and possibly momentum) which makes it different. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2586 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVu7WI8ULHxvJxGuLSnp7J_F3u8SLOks5uMNc1gaJpZM4VpQeF> .

[danpovey]

OK, thanks. Merging.