Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken DeconvolutionLayer GPU backward caused by ND conv typo #3095

Merged
merged 1 commit into from
Sep 20, 2015

Conversation

longjon
Copy link
Contributor

@longjon longjon commented Sep 20, 2015

#2049 broke deconvolution GPU backward with a typo (https://github.com/BVLC/caffe/pull/2049/files#diff-5911f1d28796ada5f80449e8bd05d270R54 omits the n * coefficient). This PR fixes.

This should really really be covered by the tests, but this PR is just the fix for master.

@longjon longjon added the bug label Sep 20, 2015
@jeffdonahue
Copy link
Contributor

Yikes! Thanks for the fix. Looking quickly (since I want to merge ASAP) I don't understand why the buggy version passes the current gradient checks...it seems like they shouldn't pass unless we were only testing the n == 1 case, which we're not -- the tests have n == 2.

jeffdonahue added a commit that referenced this pull request Sep 20, 2015
Fix broken DeconvolutionLayer GPU backward caused by ND conv typo
@jeffdonahue jeffdonahue merged commit 9dca961 into BVLC:master Sep 20, 2015
@shelhamer
Copy link
Member

@jeffdonahue @longjon It is clearly good that the indexing of the top diff is fixed so that deconv is correct in all cases, but oddly enough the code was correct as it was when doing backprop to both params and bottoms according to param_propagate_down and propagate_down. This case hinges on passing this->param_propagate_down_[0] as the skip_im2col arg of forward_gpu_gemm: in this way the weight gradient call first im2cols the right part of the top diff according to top_diff + n * this->top_dim_ and then the bottom gradient call ignores its own input arg in favor of the cached im2col result. As the gradient checker backprops to weights and bottom(s) in CheckGradientExhaustive() this condition always holds during the tests.

All-in-all this was subtle and weird but not completely broken. However, we should consider test coverage of different param_propagate_down and propagate_down settings since these could have caught this and the concat partial backprop problem #2972 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants