Added functors to make working with parameters of bijectors easier

[torfjelde]

The people have spoken: they want functors, so here there be functors.

Also, it makes it easy to reconstruct Bijector with different parameters, e.g.

julia> using Bijectors, Functors

julia> using Bijectors: PlanarLayer

julia> b = PlanarLayer(2)
PlanarLayer{Array{Float64,1},Array{Float64,1}}([-0.12509162872912338, -0.8870583749347027], [1.035763283173933, 1.6716376510180833], [0.4507888264643853])

julia> fmap(x -> x .* 10, b)
PlanarLayer{Array{Float64,1},Array{Float64,1}}([-1.2509162872912338, -8.870583749347027], [10.35763283173933, 16.716376510180833], [4.507888264643853])

[codecov]

Codecov Report

Merging #143 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #143   +/-   ##
=======================================
  Coverage   54.98%   54.98%           
=======================================
  Files          27       27           
  Lines        1726     1726           
=======================================
  Hits          949      949           
  Misses        777      777           

Impacted Files	Coverage Δ
src/Bijectors.jl	68.83% <ø> (ø)
src/bijectors/composed.jl	85.91% <ø> (ø)
src/bijectors/coupling.jl	58.13% <ø> (ø)
src/bijectors/leaky_relu.jl	94.28% <ø> (ø)
src/bijectors/named_bijector.jl	69.41% <ø> (ø)
src/bijectors/normalise.jl	75.00% <ø> (ø)
src/bijectors/planar_layer.jl	100.00% <ø> (ø)
src/bijectors/radial_layer.jl	100.00% <ø> (ø)
src/bijectors/scale.jl	36.66% <ø> (ø)
src/bijectors/shift.jl	71.42% <ø> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3387a40...14fa067. Read the comment docs.

[cpfiffer]

Slick, easy, no comments. LGTM once you bump the version.

[devmotion]

Is it possible to add some tests?

[torfjelde]

Is it possible to add some tests?

Got a suggestion? Not sure how useful it is to add a test to check if, say, Functors.functor works, or whatever. Even if it returns something empty, then that might be what it's supposed to do...

[devmotion]

For instance, in KernelFunctions we check that params returns the correct values. In GPLikelihoods we test that the keys of first \circ Functors.functor are correct.

[torfjelde]

But this has to be done on a case-by-base basis, right? Not quite clear how we can do this in general.

[cpfiffer]

Maybe one easy test would be to just check whether the parameters in PlanarLayer actually double.

[cpfiffer]

Super sophisticated tests for this are probably not worth it -- we just need to check some very basic functionality. Functors.jl bears a lot more of the onus for testing all the cases.

[devmotion]

But this has to be done on a case-by-base basis, right? Not quite clear how we can do this in general.

Hmm you mean that it has to be tested for every bijector for which we define functor? Why would this be a problem?

[torfjelde]

Hmm you mean that it has to be tested for every bijector for which we define functor? Why would this be a problem?

Yeah, exactly. Well, it's a "problem" because currently we just list a bunch of bijectors which are then iterated through and tested. So we'd have to add a new suite for which we go through a bunch of different combinations.

Now, that's of course possible. It's just that I'm uncertain how useful it is. Imagine a PR being made for a new bijector. The only way a test using Functors.functor fails is if the user forgot to add @functor CustomBijector, in which case they also didn't add the test. So in this case the test-suite won't help us find the issue.

If we already have an added test, what are the possible ways they can fail?

• We delete the @functor lines
• Functors.jl isn't working correctly
• ???

The first should never happen. The second should be caught in Functors.jl rather than here. Are there more ways it could fail?

Btw, I'm well aware that I could have added the tests in the time it took me to write this comment 😅 So it's not about whether or not I'm bothered to do it, it's just that if it doesn't actually add anything to the test-suite, IMO it shouldn't be there. I've recently made some efforts towards making it possible to test a new Bijector by simply calling a function like test_bijector, hence I'm not a big fan of adding case-by-case tests if they have no actual utility.

Do prove me wrong though! I might be missing something.

[devmotion]

IMO the main point of adding tests for functor would be to

• avoid that the lines are deleted
• check custom implementations of Functors.functor

Without tests, the first case might happen silently when refactoring the code. Custom implementations of Functors.functor are needed if not all fields are parameters (in this case the reconstruction does not work if @functor is used).

I like the test_bijector approach, we try to use the same in JuliaGaussianProcesses. The tests of Functors.functor can be integrated in such an approach, see, e.g., https://github.com/JuliaGaussianProcesses/GPLikelihoods.jl/blob/c64aa197dc5b2985949f0c74bbb85b3dd820df91/test/test_utils.jl#L23-L28. However, regarding the second point, actually I think it would be good to add a general test of the reconstruction part as well such as

xs, re = functor(my_bijector)
@test my_bijector == re(xs)

[torfjelde]

Ah, testing the reconstruction is a great idea 👍 When I last used Flux.params (which used @functor under the hood), I never use the reconstruction-functionality (might not have existed at that point though), hence I didn't think of that. But I'm very for testing that, i.e. add the lines you suggested 👍

[torfjelde]

Closed in favour of #162