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

Implement Functors.functor #162

Merged
merged 2 commits into from Jan 16, 2021
Merged

Implement Functors.functor #162

merged 2 commits into from Jan 16, 2021

Conversation

devmotion
Copy link
Member

This PR is an alternative to #161. As mentioned there, Flux.@treelike does not exist anymore. Instead, Flux relies on Functors.functor and reexports Flux.@functor.

This PR implements Functors.functor for bijectors and normalizing flows. Thus users can just call Flux.params without implementing @functor for the different layers.

A dependency on the lightweight package Functors.jl is added and the optional dependency on Flux is dropped.

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

This is great stuff!
I have one comment about the testing; it seems like we currently only test a couple of the bijectors?

Comment on lines +197 to +201
function test_functor(x, xs)
_xs, re = Functors.functor(x)
@test x == re(_xs)
@test _xs == xs
end
Copy link
Member

Choose a reason for hiding this comment

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

Is this being used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in the norm_flows tests.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, missed that 👍

@devmotion
Copy link
Member Author

Yes, indeed, so far only InvertibleBatchnorm, PlanarLayer, and RadialLayer are tested in norm_flows.jl. I am still a bit confused about the test setup - where would be the best place to add tests for other bijectors? Initially I assume that most interface tests would be handled by test_bijector but it seems it is only used in one single test?

@torfjelde
Copy link
Member

where would be the best place to add tests for other bijectors? Initially I assume that most interface tests would be handled by test_bijector but it seems it is only used in one single test?

That's correct, but the idea is to start using that approach for all bijectors. So feel free to add it there for now :)

@torfjelde
Copy link
Member

Actually, I guess it's somewhat non-trivial to add this to test_bijector, as we don't have access to the parameters of the bijector itself?

@devmotion
Copy link
Member Author

devmotion commented Jan 11, 2021

I assume one would have to specify the parameters explicitly, similar to test_functor. However, one could set them to nothing by default and skip the tests in this case.

@torfjelde
Copy link
Member

E.g.

function test_bijector(
    b::Bijector,
    xs_true,
    ys_true,
    logjacs_true
    params = nothing;
    kwargs...
)
    !isnothing(params) && test_functor(b, params)
    return test_bijector(b, xs_true, ys_true, logjacs_true; kwargs...)
end

?

@devmotion
Copy link
Member Author

This will lead to a stackoverflow error. But in principle this is the idea.

@torfjelde
Copy link
Member

Oh yeah, rather:

function test_bijector(
    b::Bijector,
    xs_true,
    ys_true,
    logjacs_true
    params;
    kwargs...
)
    test_functor(b, params)
    return test_bijector(b, xs_true, ys_true, logjacs_true; kwargs...)
end

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

I'm happy for now, and we can make it testing the functor functionality part of the tests once we unify testing a bit.

@devmotion
Copy link
Member Author

Ah sorry, I forgot about the PR. OK, let's update the tests separately then!

@devmotion devmotion merged commit c8d622f into master Jan 16, 2021
@devmotion devmotion deleted the dw/functors branch January 16, 2021 10:42
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.

None yet

2 participants