Improve keyword argument handling in @compact

[gaurav-arya]

This PR fixes issues with the keyword argument handling of @compact, noted in #12 (comment).

CC @mcabbott @MilesCranmer

Attempts to break are most welcome 🙃 Note that this does not address all the points brought up in #12 (comment), only the ones related to keyword arguments.

(NB: there's some noise in the test file due to a7d0851, you may wish to exclude it from the diff when reviewing).

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.50% 🎉

Comparison is base (9dcae27) 78.64% compared to head (f84e34a) 79.14%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   78.64%   79.14%   +0.50%     
==========================================
  Files           6        6              
  Lines         206      211       +5     
==========================================
+ Hits          162      167       +5     
  Misses         44       44              

Files Changed	Coverage Δ
src/compact.jl	92.77% <100.00%> (+0.46%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gaurav-arya]

@mcabbott I saw you added a fourth case to #12 (reply in thread). I don't think that affects this PR though, which just fixes keyword arguments.

[mcabbott]

Would be better not to hit Base.in. Also, what's gained by storing this, or indeed storing act=relu?

Suggested change

	in = 5
	out = 7
	d = @compact(; in, out, W=randn(out, in), b=zeros(out), act=relu) do x
	d_in, d_out = 3, 7
	layer = @compact(W=randn(d_out, d_in), b=zeros(d_out), act=relu) do x

And maybe instead show how to make the equivalent Flux layer?

layer([1,2,3]) ≈ Dense(layer.variables.W, zeros(7), relu)([1,2,3])  # 7-element Vector

[gaurav-arya]

Done. (The act is indeed not necessary, but it's just a matter of style and since it's harmless I didn't want to mess with it here)

[mcabbott]

This logic handles the first case below, but can it be done nicely on all arguments to handle also the second case here?

julia> macro pr(exs...)
         @show exs; nothing
       end
@pr (macro with 1 method)

julia> @pr(; a=1, b) do x
         x+a+b
       end
exs = (:((x,)->begin
          #= REPL[57]:2 =#
          x + a + b
      end), :($(Expr(:parameters, :($(Expr(:kw, :a, 1))), :b))))

julia> @pr(x -> x+a+b; a=1, b)
exs = (:($(Expr(:parameters, :($(Expr(:kw, :a, 1))), :b))), :(x->begin
          #= REPL[58]:1 =#
          x + a + b
      end))

julia> @pr(x -> x+a+b, a=1, b=2)
exs = (:(x->begin
          #= REPL[59]:1 =#
          x + a + b
      end), :(a = 1), :(b = 2))

Or might it be better to have the function call itself with arguments in re-arranged order?

[gaurav-arya]

Done. I added

Fluxperimental.jl/src/compact.jl

Lines 125 to 126 in 9ab4b87

	@warn "function stringifying does not yet handle all cases, using empty string"
	""

just so that the test could run, it should not stay very long hopefully:)

[gaurav-arya]

Oops. That warning isn't sufficient because this PR doesn't have the new logic in #17. So I addressed this comment there instead, in c1f6605 (sorry for the mess...)

[mcabbott]

Ok. So we merge this, and work on any further refinements in #17?

[gaurav-arya]

Yes, that sounds good.

[mcabbott]

One question is whether this counts as a breaking change. This used to work, and the doc example did something similar, so probably it is?

julia> @compact(n=1, w=rand(n)) do x
         x .+ w
       end
ERROR: UndefVarError: `n` not defined

[gaurav-arya]

Yes, it is a breaking change -- I indeed did have to change the linear activation example, which previously defined in and out only inside the macro and did W = randn(out, in).

[mcabbott]

Can you edit Project.toml, and then we merge?