Added nnlib layers and test cases, added upsampling layers#38
Added nnlib layers and test cases, added upsampling layers#38bors[bot] merged 2 commits intoFluxML:masterfrom
Conversation
DhairyaLGandhi
left a comment
DhairyaLGandhi
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
| Tensor{T,N}(ptr[], on(t)) | ||
| end | ||
|
|
||
| function _maxpool(t::Tensor{T,N}, kernel_size; stride = [1], padding = [0], dilation = [1]) where {T,N} |
There was a problem hiding this comment.
Why do we need this method?
There was a problem hiding this comment.
I did not have the intention at first to make a PR, so I changed a few things that was not really needed. Sorry for that.
In my mind it is more clean to have the NNlib ConvDims and PoolDims only in nnlib.jl and not in the ops.jl. Also it makes sense to align the arguments of _meanpool and _maxpool. My own preference would be to remove the _maxpool method with PoolDims.
There was a problem hiding this comment.
Best to keep the consistency with the method signatures and API for now.
There was a problem hiding this comment.
I was not aware of the #21 issue. Is this PR still interesting? I can clean it up to include only the tests and the changes needed to pass the tests. In other words, remove the added layers, leave the method signatures as they are. Reversing strides, pads, dilations... to match the spatial sizes in ops.jl is however still needed.
I guess this will be resolved by #21 at some point.
There was a problem hiding this comment.
The PR is definitely interesting. #21 should be fixed in the latest release too.
Keeping the signatures consistent is important however. The layers themselves should be fine. Reversing the dims is right too, I think.
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild succeeded: |
|
I think we can do some of the changes in future PRs, thanks @bjosv79 ! bors r+ |
|
Regarding method signatures, I assumed that any kwargs sent to NNlib for pooling and conv, eventually results in a NNlib call using ConvDims/PoolDims. If that is a correct assumption, it seems easier to let NNlib process the kwargs and only provide methods without kwargs. |
|
Build succeeded: |
|
Following the method signatures defined by packages is best since that means there is a clear future deprecation path, as well as maintenance of backwards compatibility by dependencies is easier. |
3 participants
Added nnlib layers and layer tests (not exhaustive)
Fixed issues revealed by nnlib tests
Added upsampling layers