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

Adding a Link object #14

Merged
merged 22 commits into from
Jul 21, 2021
Merged

Adding a Link object #14

merged 22 commits into from
Jul 21, 2021

Conversation

theogf
Copy link
Member

@theogf theogf commented Sep 6, 2020

We need to support different type of mapping when a constraint is applied on the GP, e.g. for Poisson the given standard was exp(f) but there are more existing options : f^2, or s * logistic(f)

The convention is to call the connection between the parameter(s) m needed for the likelihood and the input(s) f the inverse link. Cause traditionally the link would be the function to go from m to f. See for example : http://web.pdx.edu/~newsomj/mvclass/ho_link.pdf

This PR aims at introducing this with AbstractLink type.
Some likelihoods need an inverse link, for example BernoulliLikelihood or PoissonLikelihood. So it makes sense to have these likelihoods contain the inverse link invlink and parametrize it. This allows furthermore to differentiate different overloads later and for people to use their own transformations. For example my augmentation stuff only works for the LogitLink but not the ProbitLink.

For a small collection of AbstractLinks I defined just apply(link, x) (which is called by (link::LinkType)(x). and also Base.inv which returns the existing inverse link if it exists (similarly to Bijectors).

I already did a basic implementation but I realised a lot of it is overlapping with Bijectors.jl and we could consider using them. However this is also a heavier dependency ? I think NNlib is pretty heavy right?

@devmotion
Copy link
Member

An alternative would be to not use different link functions but define additional likelihood functions (i.e., one for Poisson with the exp link, one with x^2 etc.).

@theogf
Copy link
Member Author

theogf commented Sep 6, 2020

I thought that it would be cooler to be able to input really any kind of link! One could even think about passing a constrained NN (paper idea here 😆 )

@theogf theogf mentioned this pull request Sep 7, 2020
Copy link
Member

@sharanry sharanry left a comment

Choose a reason for hiding this comment

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

Is it possible to keep links separated from the likelihoods? That is, not have it part of every likelihood? We could think of this as an optional step between the GP outputs and likelihood function.

We could also define LinkedLikelihood similar to transforms in KernelFunctions.jl such that it is just another likelihood.

struct LinkedLikelihood{T}
link::Link
lik::T
end

(ll::LinkedLikelihood)(f::Real) = ll.lik(ll.link(f))
(ll::LinkedLikelihood)(fs::AbstractVector{<:Real}) = ll.lik(ll.link.(fs))

or something like

struct LinkedLikelihood{T1,T2}
link::T1
lik::T2
end

(ll::LinkedLikelihood)(f::Real) = ll.lik(ll.link(f))
(ll::LinkedLikelihood)(fs::AbstractVector{<:Real}) = ll.lik(ll.link.(fs))

I am supportive of the second approach as both links and likelihoods are essentially functions. And the second approach provides the maximum amount of flexibility.

src/likelihoods/gaussian.jl Show resolved Hide resolved
src/likelihoods/link.jl Outdated Show resolved Hide resolved
@theogf
Copy link
Member Author

theogf commented Sep 7, 2020

Is it possible to keep links separated from the likelihoods? That is, not have it part of every likelihood? We could think of this as an optional step between the GP outputs and likelihood function.

We could also define LinkedLikelihood similar to transforms in KernelFunctions.jl such that it is just another likelihood.

I don't think it is possible to separate links and likelihoods. Unlike in KernelFunctions, for some Likelihoods links/transformations are necessary. I like the idea of LinkedLikelihood to have link for Likelihood which don't need a Link but for which it could be interesting to have one.

@sharanry
Copy link
Member

sharanry commented Sep 7, 2020

Unlike in KernelFunctions, for some Likelihoods links/transformations are necessary.

Okay that makes sense. 🙂 But we should always have a sensible default link in such cases.

@theogf
Copy link
Member Author

theogf commented Sep 7, 2020

Actually one change that could be need is the naming. Should it be InvLink or Link? For example in GPFlow they use invlink : https://github.com/GPflow/GPflow/blob/5a945d67b37120610880c3323224a4e86404ae1d/gpflow/likelihoods/scalar_discrete.py#L16
Additionally here: https://en.wikipedia.org/wiki/Generalized_linear_model#Link_function, the link is defined as logit and not logistic. Never really understood why btw...

@willtebbutt willtebbutt mentioned this pull request Sep 22, 2020
@theogf
Copy link
Member Author

theogf commented Apr 14, 2021

As an alternative (I feel I am rewriting existing code) we could use Bijectors.jl To create the links?
I think for the most important ones, there exist a bijector and its inverse for it.

@devmotion
Copy link
Member

IMO we shouldn't use Bijectors.jl, mainly because it's a very heavy dependency and, more importantly, since my impression is that its design is very much focused on Turing and AdvancedHMC. In particular, all inputs and outputs have to be arrays and have to be of the same dimension and size. IMO this is quite limiting in more general applications where you want to map a submanifold of lower dimension (e.g. matrices with a special structure or the simplex - there are some open issues but I don't think the design will be changed in the near future since it would be very breaking for Turing).

However, I also think it would be good to use existing functionality in other packages, if possible. Maybe TransformVariables could be helpful? IIRC it supports more flexible mappings and input and output types and would be a lighter dependency.

@theogf
Copy link
Member Author

theogf commented Apr 14, 2021

I had a look at TransformVariables and it looks great indeed, only I think we would need to use the low level API the as API is too limiting. Funnily enough they have a transform function 😂. Sounds like the change for KernelFunctions.jl comes right in time!

src/GPLikelihoods.jl Outdated Show resolved Hide resolved
@st--
Copy link
Member

st-- commented Apr 19, 2021

Actually one change that could be need is the naming. Should it be InvLink or Link? For example in GPFlow they use invlink : https://github.com/GPflow/GPflow/blob/5a945d67b37120610880c3323224a4e86404ae1d/gpflow/likelihoods/scalar_discrete.py#L16
Additionally here: https://en.wikipedia.org/wiki/Generalized_linear_model#Link_function, the link is defined as logit and not logistic. Never really understood why btw...

Depends on which way around you define the bijector. In the statistics community, the link function is well defined to be the object that satisfies link(y) = f, where y is your observations/likelihood parameters, and f is your latent (generally assumed linear) model. So y = invlink(f). In implementing likelihoods for our GP models we generally care about the f -> y direction only, so we define the invlink explicitly.

@theogf
Copy link
Member Author

theogf commented Apr 19, 2021

@st-- Thanks for the explanation. I was indeed not familiar with the statistics nomenclature. The names have been changed accordingly already. One can switch from one representation to another (when possible) with inv

@theogf
Copy link
Member Author

theogf commented Jul 7, 2021

Can I get a fresh review on this @devmotion @st-- @willtebbutt ? Happy to get it back from the deads

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Just done a quick pass.

src/GPLikelihoods.jl Outdated Show resolved Hide resolved
src/GPLikelihoods.jl Show resolved Hide resolved
src/likelihoods/bernoulli.jl Show resolved Hide resolved
src/likelihoods/categorical.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2021

Codecov Report

Merging #14 (e308d72) into master (c46ad70) will increase coverage by 12.97%.
The diff coverage is 85.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #14       +/-   ##
===========================================
+ Coverage   69.56%   82.53%   +12.97%     
===========================================
  Files           6        7        +1     
  Lines          23       63       +40     
===========================================
+ Hits           16       52       +36     
- Misses          7       11        +4     
Impacted Files Coverage Δ
src/likelihoods/gamma.jl 60.00% <66.66%> (+10.00%) ⬆️
src/likelihoods/bernoulli.jl 75.00% <75.00%> (+8.33%) ⬆️
src/likelihoods/exponential.jl 75.00% <75.00%> (+8.33%) ⬆️
src/likelihoods/poisson.jl 75.00% <75.00%> (+8.33%) ⬆️
src/likelihoods/gaussian.jl 77.77% <87.50%> (+6.34%) ⬆️
src/links.jl 87.87% <87.87%> (ø)
src/likelihoods/categorical.jl 100.00% <100.00%> (ø)

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 c46ad70...e308d72. Read the comment docs.

@theogf
Copy link
Member Author

theogf commented Jul 21, 2021

@st-- @willtebbutt
Ready for another pass

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Broadly looks really nice -- a number of style things / comments on docstrings, and a couple of small design things.

src/likelihoods/categorical.jl Outdated Show resolved Hide resolved
src/likelihoods/categorical.jl Outdated Show resolved Hide resolved
src/likelihoods/exponential.jl Outdated Show resolved Hide resolved
src/likelihoods/gamma.jl Outdated Show resolved Hide resolved
src/likelihoods/gaussian.jl Outdated Show resolved Hide resolved
src/links.jl Outdated Show resolved Hide resolved
src/links.jl Outdated Show resolved Hide resolved
src/links.jl Outdated Show resolved Hide resolved
src/links.jl Outdated Show resolved Hide resolved
test/links.jl Show resolved Hide resolved
Copy link
Member

@willtebbutt willtebbutt 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 with this now.

@theogf theogf merged commit 56531dd into master Jul 21, 2021
@theogf theogf deleted the create_link branch October 14, 2021 08:35
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

6 participants