Functors.jl icon indicating copy to clipboard operation
Functors.jl copied to clipboard

functor by default

Open CarloLucibello opened this issue 3 years ago • 8 comments

Makes everything a functor by default, avoiding the need to sprinkle

@functor T

everywhere in Flux's layers and similar use cases. The types already decorated with @functor T or @functor T (a, b) won't be affected by the change.

The amount of breakage and unintended consequence this PR could produce is something I cannot estimate at the moment.

Fix #49

TODO:

  • [x] docs
  • [x] run Optimisers.jl tests on this PR
  • [x] run Flux.jl tests on this PR

CarloLucibello avatar Nov 25 '22 07:11 CarloLucibello

I think Kyle said he had a branch doing something like this, using ConstructionBase. That appears to be able to reconstruct closures which is neat:

julia> adder = let y = ones(1)
                 x -> x .+ y
               end
#38 (generic function with 1 method)

julia> adder.y
1-element Vector{Float64}:
 1.0

julia> adder(2)
1-element Vector{Float64}:
 3.0

julia> using ConstructionBase

julia> newadder = constructorof(typeof(adder))([4 5 6])
#38 (generic function with 1 method)

julia> newadder(2)
1×3 Matrix{Int64}:
 6  7  8

It is happy to re-build things like reshape(rand(Int8, 4)',2,2), not sure how that will interact with fmap(f, x, dx) for gradients etc.

mcabbott avatar Nov 26 '22 18:11 mcabbott

I learned recently that it's possible to de- and reconstruct closure types. See https://github.com/JuliaGPU/Adapt.jl/pull/58 for an implementation of this. Even if we can't functor all user-defined types by default, maybe this is something we could in a backwards-compatible way? I could see it as a pilot project of sorts too.

ToucheSir avatar Jan 21 '23 01:01 ToucheSir

With Flux v0.15 in preparation I think this is the right time to merge this.

CarloLucibello avatar Oct 21 '24 07:10 CarloLucibello

I marked as leaves

  • Number
  • AbstractArray{<:Number}
  • AbstractString

Other suggestions?

CarloLucibello avatar Oct 21 '24 15:10 CarloLucibello

Would be good for docs to note that not every AbstractArray of Numbers is a leaf, e.g. Transpose

mcabbott avatar Oct 21 '24 16:10 mcabbott

If I can get an approval I would go on and merge this

CarloLucibello avatar Oct 27 '24 06:10 CarloLucibello

I added some benchmarks. It seems that we have some regressions with respect to master, although not on flux types (I don't know why). I'm not sure how much we should care about this performance regression since the absolute values seem reasonable. If we do care we can try another implementation with @generated, cf https://github.com/FluxML/Functors.jl/pull/51#discussion_r1818274933, but not in this PR.

cl/fun master cl/fun/master
fmap/concrete struct 0.458 ± 0.001 μs 0.125 ± 0.041 μs 3.66
fmap/flux dense 0.5 ± 0 μs 0.5 ± 0.042 μs 1
fmap/flux dense chain 1.46 ± 0.042 μs 1.42 ± 0.042 μs 1.03
fmap/named tuple 0.292 ± 0.042 μs 0.334 ± 0.042 μs 0.874
fmap/non-concrete struct 0.542 ± 0.041 μs 0.125 ± 0 μs 4.34
time_to_load 0.059 ± 0.0021 s 0.0518 ± 0.0054 s 1.14

CarloLucibello avatar Oct 28 '24 07:10 CarloLucibello

Can this update the readme too? I think that (besides deleting @functor Foo) it wants a big notice that [email protected] is sort-of opt-out now, defaults to calling ConstructionBase.

mcabbott avatar Oct 28 '24 17:10 mcabbott

let's unleash havoc on the world! ahah, hopefully not

CarloLucibello avatar Nov 01 '24 08:11 CarloLucibello