LatentGP API: approximate posteriors & wrapping ApproxPosteriorGP

[st--]

And/or should we have a separate LatentApproxPosteriorGP?...

[willtebbutt]

To my mind ApproxPosteriorGP ought to store only what it needs in order to construct the GP corresponding to the approximate posterior. If it needs to hold onto the likelihood for some reason in order to implement the AbstractGPs API, that's fine, but I wouldn't be in favour of allowing users to have access to it.

I would be favour of posterior(::LatentGP) producing another LatentGP-like object (possibly a LatentGP containing an ApproxPosteriorGP or a new type if that doesn't work out for some reason). In this kind of object, one would keep everything.

[st--]

You're right, we can just construct LatentGP(ApproxPosteriorGP(...), ...) - e.g. as follows (excerpted from https://github.com/JuliaGaussianProcesses/ApproximateGPs.jl/pull/59/files#diff-2581bfaaa21433d043e4343891c3547bbba68f31c35416bd6fcc9773e256c673R333):

function AbstractGPs.posterior(la::LaplaceApproximation, lfx::LatentFiniteGP, ys)
    cache = # ...
    f_post = ApproxPosteriorGP(la, lfx.fx, cache)
    return LatentGP(f_post, lfx.lik, lfx.fx.Σy)
end

The one really annoying thing is that LaplaceGP requires an explicit jitter argument, and it seems weird to have to access lfx.fx.Σy - which should be an implementation detail this piece of code shouldn't need to know about... what do you think?

[willtebbutt]

The one really annoying thing is that LaplaceGP requires an explicit jitter argument, and it seems weird to have to access lfx.fx.Σy - which should be an implementation detail this piece of code shouldn't need to know about... what do you think?

That's a good point -- it is annoying. IIRC this is just a limitation of our abstractions at the minute. I think we can be fairly confident that lfx.fx.Σy does indeed represent a jitter term rather than anything more complicated, so hopefully it's okay...

[willtebbutt]

When I say abstraction, I think I should really say it's an implementation detail. In particular, we implement the LatentFiniteGP in terms of a FiniteGP and a likelihood, and the jitter is placed inside the FiniteGP. Probably we should just make the LatentFiniteGP have all four fields that it needs (latent process, inputs, jitter, and likelihood) to make things more explicit and to avoid depending on implementation details of the FiniteGP.

I think it probably is fine to write what you've written above though, because I think we can be confident that lfx.fx.Σy represents jitter.

[st--]

How about adding a constructor along the pseudocode lines of

function update_latent_posterior(latent::Union{LatentGP, LatentFiniteGP}, new_f_posterior::ApproxPosteriorGP)
    return LatentGP(new_f_posterior, latent.lik, latent[.fx].Σy
end

to hide the implementation detail for now?

[st--]

Oh, the other part that's missing is that the only API implemented for LatentGP so far is rand, but of course most of the time we do actually want to handle the latent GP itself; should we recommend directly accessing .f[x] for that, or provide some kind of "getter", e.g. latent(lf) = lf.f?

[st--]

Probably we should just make the LatentFiniteGP have all four fields that it needs (latent process, inputs, jitter, and likelihood) to make things more explicit and to avoid depending on implementation details of the FiniteGP.

That would require duplicating a whole bunch of methods though, instead of simply being able to call mean(my_latent_gp(x).fx)

[willtebbutt]

to hide the implementation detail for now?

Sounds good to me.

Oh, the other part that's missing is that the only API implemented for LatentGP so far is rand, but of course most of the time we do actually want to handle the latent GP itself; should we recommend directly accessing .f[x] for that, or provide some kind of "getter", e.g. latent(lf) = lf.f?

I think this makes seense. IIRC there are three that we might want: the latent, latent passed through the inverse link function, and the marginal distribution in the observed space.

I'm not sure whether we want to implement them on FiniteLatentGPs or LatentGPs -- probably working through some examples and seeing what is most useful would work.

That would require duplicating a whole bunch of methods though, instead of simply being able to call mean(my_latent_gp(x).fx)

Good point. The current implementation is probably fine.