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

allow type of x and w to differ in conv #63

Merged
merged 4 commits into from
Feb 6, 2019
Merged

Conversation

jbrea
Copy link
Contributor

@jbrea jbrea commented Sep 5, 2018

Trying to address #39

@MikeInnes
Copy link
Member

Thanks! Given that the type parameters are now not used in most cases, these can be written as A::AbstractArray, B::AbstractArray; hope you don't mind making that quick change.

@jbrea
Copy link
Contributor Author

jbrea commented Oct 10, 2018

I simplified some instances, as you suggested. Do you prefer to do this replacement everywhere?
maxpool!(y::A, x::A, k; kw...) where A<:AbstractArray looks nicely succinct.

@JobJob
Copy link

JobJob commented Dec 23, 2018

IMO makes sense to make the same change everywhere relevant in conv.jl. Restricting args to be the exact same type sounds like it was just a historical accident, and similar for many AbstractArray types returns a different type (often a straight up Array) - which is relevant e.g. here:

NNlib.jl/src/conv.jl

Lines 154 to 157 in 085adb7

maxpool(x::AbstractArray, k; pad = map(_->0,k), stride = k) =
maxpool!(similar(x, pdims(size(x), k, expand(Val{length(k)}, pad),
expand(Val{length(k)}, stride))), x, k, pad = expand(Val{length(k)}, pad),
stride = expand(Val{length(k)}, stride))

@MikeInnes
Copy link
Member

Agreed. I'll merge this once tests pass, and I'm happy to take more patches to clean this up.

@codecov-io
Copy link

codecov-io commented Feb 6, 2019

Codecov Report

Merging #63 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #63   +/-   ##
======================================
  Coverage    71.4%   71.4%           
======================================
  Files           9       9           
  Lines         619     619           
======================================
  Hits          442     442           
  Misses        177     177
Impacted Files Coverage Δ
src/conv.jl 66.66% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a470e99...b696529. Read the comment docs.

@MikeInnes MikeInnes merged commit ccc6dad into FluxML:master Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants