Mv cleanup #3008
Mv cleanup #3008
Conversation
pymc3/gp/gp.py
Outdated
| @@ -418,15 +417,15 @@ def marginal_likelihood(self, name, X, y, noise, is_observed=True, **kwargs): | |||
| if not isinstance(noise, Covariance): | |||
| noise = pm.gp.cov.WhiteNoise(noise) | |||
| mu, cov = self._build_marginal_likelihood(X, noise) | |||
| chol = cholesky(stabilize(cov)) | |||
| cov = stabilize(cov) | |||
There was a problem hiding this comment.
I'm not sure the stabilisation is needed here since noise is already being added to the diagonal in _build_marginal_likelihood.
pymc3/gp/gp.py
Outdated
| @@ -959,7 +956,7 @@ def _build_conditional(self, Xnew, pred_noise, diag): | |||
| cov = Km - Asq | |||
| if pred_noise: | |||
| cov += sigma * np.eye(cov.shape) | |||
| return mu, cov | |||
| return mu, stabilize(cov) | |||
There was a problem hiding this comment.
Ditto, actually almost certain this is redundant. (Moved from line 999, BTW)
Maybe not redundant when pred_noise == False.
pymc3/distributions/timeseries.py
Outdated
| self.nu = nu = tt.as_tensor_variable(nu) | ||
| self.init = init |
|
@gBokiau Looks great, I think iterative changes are the way to go. Definitely want @aseyboldt and @bwengals to take a close look. |
pymc3/distributions/timeseries.py
Outdated
|
|
||
| self.mu = mu = tt.as_tensor_variable(mu) | ||
| self.init = init | ||
| self.mean = tt.as_tensor_variable(0.) | ||
| self.innovArgs = (self.mu, cov, tau, chol, lower) | ||
| self.innov = multivariate.MvNormal.dist(*self.innovArgs) | ||
|
|
||
| def logp(self, x): |
There was a problem hiding this comment.
This only works if x has rank 1, right? Maybe we should intercept the shape in __init__ (one of the kwargs) and throw an error if that doesn't have length 1, just to give people a better error message.
There was a problem hiding this comment.
Er, wait, I would hope not, wouldn't rank 1 defeat the purpose of Mv?
I'll run the demo to get a better grasp. More tests for the timeseries would be nice wrt shapes #2859 (comment)
There was a problem hiding this comment.
I checked to be sure: this only accepts x and cov/chol with ndim 2, and correctly controls that the shapes are compatible.
There was a problem hiding this comment.
Related observations:
- To be more precise, it controls/catches any glitch between shape and cov upon sampling, not at declaration time. That is also the behaviour of MvN/MvT. While that could be improved, I think it should then happen in those classes.
- Another question entirely (for later, perhaps) is wether we want to intercept any such errors here, as to make them more contextual for the timeseries classes.
However this is all probably not what your concern about the rank was?
There was a problem hiding this comment.
I should have said rank 2, not 1. I didn't actually think about the fact that mvnormal doesn't support higher rank input either, but I still think we should check this in this class. If at some point in the future we extend mvnormal, people wouldn't get an error then, but I'm not sure the results are what we want. (eg which dimension is the one with the correlations? In mvnormal it is the last one....)
Here is the code in mvnormal that I have in mind:
https://github.com/pymc-devs/pymc3/blob/master/pymc3/distributions/multivariate.py#L37
There was a problem hiding this comment.
Right, gotcha. If my understanding is correct, in the case of a random walk, ndim=1 doesn't make sense either, right? So shape can only be two-dimensional for these distributions.
So, as it stands, I would simply add this control in __init__:
if len(self.shape) != 2:
raise ValueError("Only 2 dimensions are allowed.")
Perhaps for a separate PR — when given 3 dims, we could (I think?) diff the obs separately along the time axis, concatenate/stack the thus obtained innovations to two dims, and pass that on to Mv.
|
these look good to me! |
|
Thanks @gBokiau! |
This picks up some of the most straight-forward edits from #2847. Guiding principle:
Aka DRY.
For review:
In GP it appeared that covariance matrices were being stabilised multiple times over. Perhaps this was a side-effect of not following the above principle, unless there was some ground for doing this, eg the
_build_marginals()have some way of screwing up (rendering non-positive definite) the covs even after they have first been stabilised. Please review.