Skip to content

CrossCor layer #762

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
Merged
merged 10 commits into from
May 14, 2019
Merged

CrossCor layer #762

merged 10 commits into from
May 14, 2019

Conversation

ashridh
Copy link
Member

@ashridh ashridh commented May 1, 2019 •
edited
Loading

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

@ashridh ashridh mentioned this pull request May 1, 2019
Closed
MikeInnes
MikeInnes reviewed May 1, 2019
View reviewed changes
src/layers/conv.jl Outdated
(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

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?

@ashridh
Copy link
Member Author

ashridh commented May 8, 2019

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?

@ashridh
Copy link
Member Author

ashridh commented May 8, 2019

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 @ashridh
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
@ashridh ashridh 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

@ashridh @DhairyaLGandhi @avik-pal @MikeInnes