-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Refactor convolution layer and add deconvolution layer #1615
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8d2aebc
add BaseConvolutionLayer
longjon a0e9db1
add CPU_ONLY ifdef guards to BaseConvolutionLayer
longjon e3e2f2d
rewrite ConvolutionLayer to use BaseConvolutionLayer helpers
longjon 3617352
add DeconvolutionLayer, using BaseConvolutionLayer
longjon 408133c
document DeconvolutionLayer
longjon 25c2e3f
[test] simple test for DeconvolutionLayer
longjon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be slightly more elegant to give
BaseConvolutionLayer
a privateim2col_layer_
which{Dec,C}onvolutionLayer
callForward
andBackward
on, instead of these functions? (Possibly also saving some duplicated setup logic & private variables.)Relatedly, I always kind of thought we should have a separate
BiasLayer
internally called by bothInnerProductLayer
andConvolutionLayer
, factoring out that little bit of logic, and allowing one to use biases without multiplicative weights, for whatever that's worth.Just a minor thought -- definitely doesn't need to be done here as this is nice cleanup regardless, and of course deconvolution layer will be a welcome feature.
This looks good to merge to me if you think it's ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think the
im2col
layer would probably be a bit better, and I agree with factoring out the bias, although I'd rather save those things for later PRs.Other than that, I think I ought to add some tests that at least call deconv forward and backward, and then it'll be ready.