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

make bias optional in dense layer #1314

Closed
wants to merge 2 commits into from

Conversation

AliaaRassem
Copy link

This is to make the bias in dense layer as optional parameters as in the convolution networks by using Flux.Zeros()

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable
  • Final review from @dhairyagandhi96 (for API changes).

@DhairyaLGandhi
Copy link
Member

Since Flux.Zeros <: AbstractArray, we don't need to add the union. Rather maybe just add a constructor that takes a weight and bias kwarg and let the rest of the machinery take care of the rest of the computation?

@AliaaRassem
Copy link
Author

changed bias type back to AbstractArray

Copy link
Member

@DhairyaLGandhi DhairyaLGandhi left a comment

Choose a reason for hiding this comment

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

Thanks! I've left a couple thoughts up, which should make the API easier to use.

end

@functor Dense

function (a::Dense)(x::AbstractArray)
W, b, σ = a.W, a.b, a.σ
σ.(W*x .+ b)
typeof(b) <: Zeros ? σ.(W*x ) : σ.(W*x .+ b)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to change the forward pass here, Zeros would be handled appropriately

@@ -112,15 +115,15 @@ end
Dense(W, b) = Dense(W, b, identity)

function Dense(in::Integer, out::Integer, σ = identity;
initW = glorot_uniform, initb = zeros)
return Dense(initW(out, in), initb(out), σ)
initW = glorot_uniform, initb = zeros(out))
Copy link
Member

Choose a reason for hiding this comment

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

It's odd that here we have a function for weight and a vector for bias.

The more appropriate constructor would accept both weight and bias as kwargs ( refer the conv layers)

@DhairyaLGandhi
Copy link
Member

bump

@mcabbott
Copy link
Member

mcabbott commented Nov 9, 2020

Replaced by #1379 I think.

Perhaps we should still kill the initW and initb keywords, as suggested by Dhairya here: #1314 (comment).

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