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

CrossCor layer #762

Merged
merged 10 commits into from
May 14, 2019
Merged

CrossCor layer #762

merged 10 commits into from
May 14, 2019

Conversation

ayush-1506
Copy link
Member

@ayush-1506 ayush-1506 commented May 1, 2019 •
edited
Loading

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

@ayush-1506 ayush-1506 mentioned this pull request May 1, 2019
Closed
MikeInnes
MikeInnes reviewed May 1, 2019
View reviewed changes
src/layers/conv.jl Outdated
@@ -199,6 +199,66 @@ end
(a::DepthwiseConv{<:Any,<:Any,W})(x::AbstractArray{<:Real}) where {T <: Union{Float32,Float64}, W <: AbstractArray{T}} =
a(T.(x))

"""
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
MikeInnes reviewed May 1, 2019
View reviewed changes
test/cuda/cuda.jl
@@ -36,6 +36,10 @@ c = gpu(Conv((2,2),3=>4))
l = c(gpu(rand(10,10,3,2)))
Flux.back!(sum(l))

c = gpu(CrossCor((2,2),3=>4))
l = c(gpu(rand(10,10,3,2)))
Flux.back!(sum(l))
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have some numerical tests here, e.g. comparing to a regular convolution. Also, let's have a news item for this.

@DhairyaLGandhi
Copy link
Member

Not to get ahead of myself, we'll need a News item.

@DhairyaLGandhi
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented May 8, 2019

try

Merge conflict

@DhairyaLGandhi
Copy link
Member

Could you do a quick conflict resolve?

@ayush-1506
Copy link
Member Author

Done!

@DhairyaLGandhi
Copy link
Member

bors try

bors bot added a commit that referenced this pull request May 8, 2019
@bors
Try #762:
46f54a1
@bors
Copy link
Contributor

bors bot commented May 8, 2019

try

Build succeeded

@DhairyaLGandhi
Copy link
Member

Could we add the numerical tests that @MikeInnes mentioned?

@ayush-1506
Copy link
Member Author

I've added a small test, anything specific I should add there?

@avik-pal
Copy link
Member

avik-pal commented May 8, 2019

@ayush-1506 A comparison between cross_correlation and convolution, checking the fact that both are equivalent once the weights are flipped, would be a good check IMO.

@DhairyaLGandhi
Copy link
Member

We need to assert that what we are emitting is in fact the correct output. I think @avik-pal s test sounds reasonable.

@MikeInnes
Copy link
Member

Can we also link CrossCor in the layer reference?

@avik-pal once the docs are updated, if you're happy with the tests here go ahead and merge with bors.

@avik-pal
Copy link
Member

bors r+

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>
@bors
Copy link
Contributor

bors bot commented May 14, 2019

Build succeeded

@bors bors bot merged commit 98a027a into FluxML:master May 14, 2019
@ayush-1506 ayush-1506 deleted the crosscor branch May 14, 2019 10:58
@MikeInnes
Copy link
Member

Thanks @ayush-1506!

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.
4 participants
@ayush-1506 @DhairyaLGandhi @avik-pal @MikeInnes