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

LatentGP: rename to jitter and restrict to scalar Real #305

Open
st-- opened this issue Mar 30, 2022 · 0 comments
Open

LatentGP: rename to jitter and restrict to scalar Real #305

st-- opened this issue Mar 30, 2022 · 0 comments

Comments

@st--
Copy link
Member

st-- commented Mar 30, 2022

I have to say I find it quite confusing when it's good to add a type restriction because that'll prevent errors and when it's bad to add a type restriction because it'll make it less flexible. 😅 I mean, both of those are generally true. But why that results in wanting it sometimes and not wanting it other times.

In my view it's a judgement call each time -- I also don't have a standard set of rules for when I type stuff really strictly vs not.

In this case, my feeling is that knowing that you're going to get an AbstractMatrix{<:Real} (as a GP-implementer) is very helpful -- there's a good chance that logdet, Cholesky, and \ are going to work (which is not the case for Reals, AbstractVector{<:Real}s, UniformScalings etc), I can check the size if I like, I can specialise on particular matrix types etc if I know something particular about the structure of certain matrices. So it lets me make slightly stronger assumptions about the internals that are more likely to be true, and it doesn't restrict the user-facing interface.

We can't make the same restriction for LatentGP though, as at that point we don't actually know the length of the observation vector, so the jitter Σy actually needs to be a scalar, or a vector or matrix of the right shape (it simply gets passed to FiniteGP in the construction of LatentFiniteGP)...

Indeed. I think that's fine though, because it's a different type and I'm okay with it having different semantics. We should probably rename it to jitter and restrict it to be a Real at some point...

Originally posted by @willtebbutt in #236 (comment)

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

No branches or pull requests

1 participant