{"action":"created","issue":{"url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/14","labels_url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/14/labels{/name}","comments_url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/14/comments","events_url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/14/events","html_url":"https://github.com/kaldi-asr/kaldi/pull/14","id":96730104,"number":14,"title":"update Maxout GPU implementation","user":{"login":"naxingyu","id":3320260,"avatar_url":"https://avatars.githubusercontent.com/u/3320260?v=3","gravatar_id":"","url":"https://api.github.com/users/naxingyu","html_url":"https://github.com/naxingyu","followers_url":"https://api.github.com/users/naxingyu/followers","following_url":"https://api.github.com/users/naxingyu/following{/other_user}","gists_url":"https://api.github.com/users/naxingyu/gists{/gist_id}","starred_url":"https://api.github.com/users/naxingyu/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/naxingyu/subscriptions","organizations_url":"https://api.github.com/users/naxingyu/orgs","repos_url":"https://api.github.com/users/naxingyu/repos","events_url":"https://api.github.com/users/naxingyu/events{/privacy}","received_events_url":"https://api.github.com/users/naxingyu/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2015-07-23T05:12:38Z","updated_at":"2015-07-26T07:18:53Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/kaldi-asr/kaldi/pulls/14","html_url":"https://github.com/kaldi-asr/kaldi/pull/14","diff_url":"https://github.com/kaldi-asr/kaldi/pull/14.diff","patch_url":"https://github.com/kaldi-asr/kaldi/pull/14.patch"},"body":"I add the Maxout implementation on both GPU and CPU, and updated the Maxout component in nnet2."},"comment":{"url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/comments/124952783","html_url":"https://github.com/kaldi-asr/kaldi/pull/14#issuecomment-124952783","issue_url":"https://api.github.com/repos/kaldi-asr/kaldi/issues/14","id":124952783,"user":{"login":"chenguoguo","id":11788179,"avatar_url":"https://avatars.githubusercontent.com/u/11788179?v=3","gravatar_id":"","url":"https://api.github.com/users/chenguoguo","html_url":"https://github.com/chenguoguo","followers_url":"https://api.github.com/users/chenguoguo/followers","following_url":"https://api.github.com/users/chenguoguo/following{/other_user}","gists_url":"https://api.github.com/users/chenguoguo/gists{/gist_id}","starred_url":"https://api.github.com/users/chenguoguo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chenguoguo/subscriptions","organizations_url":"https://api.github.com/users/chenguoguo/orgs","repos_url":"https://api.github.com/users/chenguoguo/repos","events_url":"https://api.github.com/users/chenguoguo/events{/privacy}","received_events_url":"https://api.github.com/users/chenguoguo/received_events","type":"User","site_admin":false},"created_at":"2015-07-26T07:18:53Z","updated_at":"2015-07-26T07:18:53Z","body":"Hi Xingyu,\r\n\r\nI went through the patch (sorry for keeping you waiting, I was a bit busy with other stuff the past few days...), the code looks plausible. Since you've added the CPU version of those functions, it might be a good idea to add test code for that as well. I also have some small style wise suggestions below:\r\n\r\nHere:\r\ncuda_calc_max_deriv(dimGrid, dimBlock, this->data_, src1.Data(), src2.Data(), Dim(), src2.Stride(), group_size);\r\n\r\nLet's use cuda_calc_group_max_deriv instead of cuda_calc_max_deriv (and also\r\nchange other related function names)?\r\n\r\nSome general suggestions:\r\n\r\n1. limit lines <= 80 characters, for example here (both code and documentation):\r\n   void MatrixBase<Real>::GroupMaxDeriv(const MatrixBase<Real> &input,\r\n                                         const MatrixBase<Real> &output) {\r\n    KALDI_ASSERT(input.NumCols() == this->NumCols() && input.NumRows() == this->NumRows());\r\n\r\n2. use consistent variable names, for example, I noticed you used intput_val,\r\n   output_val and maxv, perhaps better to use max_val instead.\r\n\r\n3. use proper indentation, for example, here the two \"const\" should be aligned:\r\n   void MatrixBase<Real>::GroupMaxDeriv(const MatrixBase<Real> &input,\r\n                                          const MatrixBase<Real> &output) {\r\n     KALDI_ASSERT(input.NumCols() == this->NumCols() && input.NumRows() == this->NumRows());\r\n\r\nGuoguo"}}