[WIP] mode parameter for Convolution/cross-convolution #71
Conversation
|
Also, there was some discussion in FluxML/Flux.jl#308 about the way to express these changes in Flux. Could you give me a final decision on it so that I can go ahead and add them to Flux? |
|
We can pass |
src/conv.jl
Outdated
| function crossconv!(y::AbstractArray{T,3}, x::AbstractArray{T,3}, w::AbstractArray{T,3}; | ||
| pad = 0, stride = 1, dilation = 1) where T | ||
| args = map(x -> reshape(x, size(x,1),1,size(x,2),size(x,3)), (y, x, w)) | ||
| crossconv!(args..., pad = (pad...,0), stride = (stride...,1), dilation = (dilation...,1)) |
There was a problem hiding this comment.
That's crosscor. It should also be a thin wrapper that doesn't need as much duplication as you've added here; if you just call conv! here with the right mode, for example, you won't need specific 2D and 3D wrappers.
There was a problem hiding this comment.
But conv! doesn't take the mode parameter in its arguments at the moment, (and you also advised me not to expose mode in conv!).
There was a problem hiding this comment.
I think it would be ok to have it in conv! (perhaps as flipkernel) but just not document it. That makes it a bit easier to support crosscor! without a lot of duplication. You can also then avoid the gradient wrappers, since we can just support flipkernel in Flux.
|
@MikeInnes I've removed all gradient wrappers and added the |
|
Ok perfect. Thanks a lot! |
|
It still uses “crossconv”, which should instead be “crosscor” |
|
Yeah that's fixed in 5727643 |
@MikeInnes Would expressing the
modeargument work in each function call work, or do I need to add anothercrosscorfunction?