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.

Sign up for GitHub

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

Jump to bottom

new cross convolution layer #423

Closed
wants to merge 11 commits into from
Closed

new cross convolution layer #423

wants to merge 11 commits into from

Conversation

ayush1999
Copy link
Contributor

This works on top of changes in FluxML/NNlib.jl#71. Implemented a CrossConv layer, which calls crossconv and crossconv! functions from NNlib.

@ayush1999
Copy link
Contributor Author

Will add tests for the layer upon approval.

@MikeInnes
Copy link
Member

Looks good to me. Can you add some quick tests that it runs through, and perhaps also a gradient check for crosscor.

MikeInnes
MikeInnes reviewed Oct 9, 2018
View reviewed changes
src/tracker/array.jl Outdated
crosscor(data(x), data(w); kw...),
Δ -> nobacksies(:crosscor,
(NNlib.∇conv_data(data.((Δ, x, w))...; kw...),
NNlib.∇conv_filter(data.((Δ, x, w))...; kw...)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs the gradient check. I guess this won't be right without flipkernel, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed that. Will add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeInnes We'd have to implement flipkernel argument for the ∇conv_data family of functions, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

@ayush1999
Copy link
Contributor Author

@MikeInnes Tests pass locally.

@MikeInnes
Copy link
Member

Sorry for dropping this for a while. Can you do a quick rebase? Will probably need the gradients part of the PR to move to Tracker.jl.

@MikeInnes MikeInnes mentioned this pull request Mar 26, 2019
Closed
@ayush1999 ayush1999 force-pushed the dev_mode branch 3 times, most recently from 2482c5c to 161fa86 Compare March 26, 2019 18:41
@ayush1999 ayush1999 mentioned this pull request Mar 27, 2019
Closed
@MikeInnes
Copy link
Member

MikeInnes commented Apr 4, 2019 •
edited
Loading

@staticfloat did crosscor dissappear as part of the NNlib overhaul?

If so, it's not necessarily an issue; we can just define it in Flux and use it the same way (AD should work automatically if it just forward to conv).

@staticfloat
Copy link
Contributor

Ah, yes, it did. It has been subsumed into conv(). So the conv() call now takes a DenseConvDims object, which you set flipkernel=true within in order to do cross-correlation.

@MikeInnes
Copy link
Member

Ok cool. In that case @ayush1999 let's just define crosscor in Flux. We won't need the Tracker PR then. You'll need the updated branch of Tracker that's compatible with the new NNlib though.

MikeInnes
MikeInnes reviewed Apr 5, 2019
View reviewed changes
src/layers/conv.jl Outdated
bias::V
stride::NTuple{N,Int}
pad::NTuple{N,Int}
dilation::NTuple{N,Int}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting is off here

@MikeInnes
Copy link
Member

Can we keep the crosscor function – it can just take a DenseConvDims and set flipkernel=true. Then the CrossCor layer can call that.

Also, can we get a GPU test for this layer as well.

@staticfloat
Copy link
Contributor

Can we keep the crosscor function – it can just take a DenseConvDims and set flipkernel=true

@ayush1999 you can set flipkernel easily by using the kwargs constructor of DenseConvDims:

new_cdims = DenseConvDims(cdims; flipkernel=true)

@ayush1999
Copy link
Contributor Author

If I'm not wrong, we'd need to tag a new release for NNlib so that the tests can pass.

@staticfloat
Copy link
Contributor

We're going to need to wait for #714 or #718 to be merged first, yes.

@MikeInnes
Copy link
Member

Those PRs are in, so we should be able to get this in now, just has one more conflict.

Note that you can also put branches of packages in the manifest to get tests to pass. Shouldn't be necessary now though.

@ayush-1506 ayush-1506 mentioned this pull request May 1, 2019
Merged
@ayush-1506
Copy link
Member

I've added all these changes in a new PR (#762). (I lost access to the github account for this PR). Can you please close this and review the new PR?

bors bot added a commit that referenced this pull request May 14, 2019
@bors @ayush-1506
Merge #762
8ee6af1 
762: CrossCor layer r=avik-pal a=ayush-1506

Same as #423 (which could be edited since I lost access to that github account).

Co-authored-by: ayush-1506 <ayush.shridhar1506@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeInnes MikeInnes MikeInnes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ayush1999 @MikeInnes @staticfloat @ayush-1506 @DhairyaLGandhi