Skip to content

Conversation

darsnack
Copy link
Member

@darsnack darsnack commented Dec 6, 2019

Based on Slack chatter, I added a utility function, outdims, that computes the output dimensions for given input dimensions.

Example

layer = Conv((3, 3), 3 => 16)
outdims(layer, (10, 10)) # returns (8, 8)

end

@testset "conv output dimensions" begin
m = Conv((3, 3), 3 => 16)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have some tests for different padding, stride and dilation

@MikeInnes
Copy link
Member

I can see this being useful. Note that not every method needs a docstring; you can just give one docstring with a couple of examples, otherwise the docs for outdims will be pretty noisy.

You can probably reuse the tools in NNlib to calculate this for convolutions, which will make it much less likely to miss anything.

@darsnack
Copy link
Member Author

darsnack commented Dec 7, 2019

Okay switched to NNlib.jl, added some more tests w.r.t. pad, stride, etc., and also added a small section to the docs.

@darsnack
Copy link
Member Author

darsnack commented Dec 8, 2019

Any ideas on the build errors? I don't really follow the stack trace, and I am not able to reproduce it on my local system.

@MikeInnes
Copy link
Member

Quite unlikely to be your fault, but we should get it passing. If you push a new commit the error might just go away.

@darsnack
Copy link
Member Author

Quite unlikely to be your fault, but we should get it passing. If you push a new commit the error might just go away.

So I guess this didn't work. Have you seen this issue pop up with any other builds since I last pushed?

@MikeInnes
Copy link
Member

We have had a bit of flakiness lately.

bors try

bors bot added a commit that referenced this pull request Jan 14, 2020
@bors
Copy link
Contributor

bors bot commented Jan 14, 2020

try

Build succeeded

@IanButterworth
Copy link
Contributor

It would be great to revive this idea. Perhaps worth rebasing and re-running tests?

@darsnack
Copy link
Member Author

@MikeInnes looks like the build is passing now? Though Travis is only testing for Julia 1.3?

@DhairyaLGandhi
Copy link
Member

bors try

Let's try this once to see if things are settled down. Post which thoughts on documenting it's scope, since it might be odd to get it running with arbitrary functions?

bors bot added a commit that referenced this pull request Feb 10, 2020
@bors
Copy link
Contributor

bors bot commented Feb 10, 2020

try

Build succeeded

@darsnack
Copy link
Member Author

Sure I can push a commit updating the doc section to specify which layers it works on?

@IanButterworth
Copy link
Contributor

Perhaps the docs could state which layers it works for out of the box, and give the outdims conv method code to give a handy starting point for people to generate an outdims method for custom layers?

@IanButterworth
Copy link
Contributor

This is looking good to me and I'm keen to start using these functions.
Is it ready to be merged?

@darsnack
Copy link
Member Author

Is it ready to be merged?

I am satisfied with merging it in the current form.

@DhairyaLGandhi
Copy link
Member

Happy to merge this now
bors r+

@bors
Copy link
Contributor

bors bot commented Feb 25, 2020

Build succeeded

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.

5 participants