Added deprecation warning to Dense(in,out,act,initW,initb)

[shreyas-kowshik]

Addresses issue #671 .
@MikeInnes is this what was needed?

[johnnychen94]

we can't just deprecate the whole method here. The tricky part is that keyword arguments are not counted in multiple dispatch.

One possible solution (not sure if it works)

1. change initW = glorot_uniform to initW = nothing
2. manual check before initialization

if initW is nothing
  initW = glorot_uniform;
else
  depwarn
end

[shreyas-kowshik]

I see...My bad...I'll fix this accordingly...

[johnnychen94]

Looks like this strategy works.

Now you need to have a look at the codebase and deprecate other functions such as Diagonal, ConvTranspose. And don't forget to modify the testcases.

BTW, it might be useful to provide a link to #671 as a comment alongside your depwarn.

[shreyas-kowshik]

I suppose that the other layers do not have this issue and this mostly pertained to the Dense layer.
Conv and ConvTranspose only have a single init in them. So I don't think that they should need deprecation.
At least this was what I could infer from #671 .
Is this all or am I getting it wrong? @MikeInnes

[johnnychen94]

It's not about single init keyword, it's about consistency. If you deprecate init as a keyword argument, deprecate them all.

[MikeInnes]

Let's get the Dense deprecation done and then do a review of other layers. I think we probably will want to get rid of all of them though.

I think the kwargs method could be defined separately. You should be able to do something like

@deprecate Dense(in::Integer, out::Integer; initW = ...) = Dense(initW(out, in), ...)

which separates the deprecated code from the new code nicely, and gives a clearer instruction to the user.

[johnnychen94]

@MikeInnes @deprecate might not work well for keyword arguments. They don't participate in dispatch.

julia> foo(a,b;k=1) = println()
foo (generic function with 1 method)

julia> @deprecate foo(a,b;k=1) foo(a,b)
foo (generic function with 1 method)

julia> foo(1,2)
┌ Warning: `foo(a, b; k=1)` is deprecated, use `foo(a, b)` instead.
│   caller = top-level scope at none:0
└ @ Core none:0

https://github.com/JuliaDocs/Documenter.jl/blob/c59fe92f880e7acfbf2610fed0c70dc364e4c3a0/src/Documenter.jl#L473-L513 is an example of such deprecation work.

[MikeInnes]

It actually works if you do it the other way around:

julia> @deprecate foo(a,b;k=1) foo(a,b)
foo (generic function with 1 method)

julia> foo(a,b;k=1) = println()
foo (generic function with 1 method)

julia> foo(1, 2)

But to be fair this probably still results in a method warning. Another option is to use a more specific signature for the non-deprecated method. It might be best just to stick with the if-then approach though.

[johnnychen94]

If it's all about deprecating Dense, then I think this PR is ready to merge. @MikeInnes
And it's better to add some comment to guide the future work. @shreyas-kowshik The content might be why we need to deprecate this or perhaps a single url link.

[johnnychen94]

I think "keyword argument `initW` is deprecated; use `Dense(W,b)` to initialize" is more descriptive to the current one.

[CarloLucibello]

closing as we are going in a different direction with #1440