Skip to content

Conversation

@mcabbott
Copy link
Member

@mcabbott mcabbott commented Sep 2, 2019

Fixes #282, by allowing Dense((5, 3), (2, 7)) as suggested. Also closes #293, #617.

For d = Dense(15, 14) I wanted to leave the original behaviour alone, but attempting to do this by dispatch landed me in ambiguity hell, so it's an if statement (which I think ought to get compiled out). Possibly this should have && ndims(x)<=2 so that such d does allow higher-dimensional data.

I also added a line to allow Dense(15 => 14) and Dense((5, 3) => (2, 7)), more like Conv and less confusing I think. But will remove if anyone objects / thinks this deserves its own issue.

Note by the way the following ::Any problem. Since this persists after ] free Flux, I don't think it's my fault. (This is on Julia 1.1)

julia> d = Dense(5,2)
Dense(5 => 2)

julia> @code_warntype d(rand(Float32, 5))
Body::Any

@DhairyaLGandhi
Copy link
Member

This is interesting! We should also add a depwarn to the changed API.

@mcabbott
Copy link
Member Author

mcabbott commented Sep 2, 2019

Glad someone likes =>! But what I propose just to leave Dense(5,2) working, so that nothing breaks. If that's not too confusing?

I can add a news line, but should it go under v0.10.0?

@mcabbott mcabbott mentioned this pull request Sep 27, 2019

"""
Dense(in::Integer, out::Integer, σ = identity)
Dense(in => out, σ = identity)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have in this docstring, or as separete docstrings, also the other constructors?

Dense(in, out)
Dense(W, b)

Copy link
Member Author

Choose a reason for hiding this comment

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

See if you like this now. I’ve also tried to make clearer that σ is not a keyword.

@mancellin
Copy link

What is the status of this?
I've been drafting a small PR with some improvements of Dense docstring, but most of it would actually be fixed (or outdated) by this PR from @mcabbott.

@mcabbott
Copy link
Member Author

mcabbott commented Mar 18, 2020

Would still be nice, IMO, but didn't gain traction?

I guess an alternative would be to make a nice Reshape() layer, this could even support the same notation if you defined e.g.
Dense((2,3) => (4,5)) = Chain(Reshape((2,3) => 6), Dense(6,20), Reshape(20 => (4,5))).

Instead of bias=false, there's also a proposal in #873 to write bias=Zeros().

@CarloLucibello
Copy link
Member

+1 for the Reshape layer, to me it seems more manageable and of broader utility

@mcabbott mcabbott closed this Apr 16, 2020
@bhvieira
Copy link
Contributor

+1 for the Reshape layer as well

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.

Support for additional dimensions in Dense layer

5 participants