Skip to content

Comments

Replaced sigma argument with noise in MarginalSparse.marginal_likelihood#2969

Merged
fonnesbeck merged 5 commits intomasterfrom
marginal_likelihood_arg
May 10, 2018
Merged

Replaced sigma argument with noise in MarginalSparse.marginal_likelihood#2969
fonnesbeck merged 5 commits intomasterfrom
marginal_likelihood_arg

Conversation

@fonnesbeck
Copy link
Member

Currently, MarginalSparse.marginal_likelihood uses a sigma argument, while Marginal.marginal_likelihood uses noise. Changed to noise to make it consistent, and deprecated sigma.

@fonnesbeck fonnesbeck requested a review from bwengals May 7, 2018 02:58
@twiecki
Copy link
Member

twiecki commented May 7, 2018

Wouldn't it be more consistent to have everything be sigma instead?

@fonnesbeck
Copy link
Member Author

Depends on what we are being consistent with. The rest of the GP module uses noise, the normal distribution uses sd. It should at least be the same within the class of models.

@bwengals
Copy link
Contributor

bwengals commented May 7, 2018

The logic for the different names was that in the general case the noise doesn't have to be diagonal, you can either give an arbitrary covariance func or just sigma to it. But for MarginalSparse it needs to be white. sd instead of sigma makes sense though. I'm ok with changing the name either way.

@fonnesbeck
Copy link
Member Author

I think the difference can be made clear in the docstrings, rather than in the naming. It felt odd having to switch the keyword argument when changing from a full to a sparse GP. noise is kind of nice because, though more colloquial than sd or sigma, it is clear that it is referring to observation noise, rather than process error. However, a case can be made for sigma and sd to make them consistent with either MvNormal or Normal respectively, though it seems that scalars are generally passed, which would favor using sd. Thoughts?

@junpenglao
Copy link
Member

I think the difference can be made clear in the docstrings, rather than in the naming.

Second this. @bwengals' explanation should definitely go to doc/docstring.

Copy link
Contributor

@bwengals bwengals left a comment

Choose a reason for hiding this comment

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

Everything OK with me

@fonnesbeck fonnesbeck merged commit 484bf65 into master May 10, 2018
@fonnesbeck fonnesbeck deleted the marginal_likelihood_arg branch May 10, 2018 16:15
@fonnesbeck fonnesbeck restored the marginal_likelihood_arg branch May 10, 2018 16:18
@junpenglao junpenglao deleted the marginal_likelihood_arg branch May 10, 2018 16:21
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.

4 participants