Fix for mean functions #2111
Fix for mean functions #2111
Conversation
pymc3/gp/cov.py
Outdated
| @@ -342,7 +337,7 @@ def square_dist(self, X, Z): | |||
| sqd = -2.0 * tt.dot(X, tt.transpose(X)) +\ | |||
| (tt.reshape(Xs, (-1, 1)) + tt.reshape(Xs, (1, -1))) | |||
| else: | |||
| Z = tt.as_tensor_variable(Z) | |||
| Z = tt.as_tensor_variable(X) | |||
There was a problem hiding this comment.
Is this intentional? If so, might need a comment.
There was a problem hiding this comment.
no not intentional! good catch...
pymc3/gp/mean.py
Outdated
| Parameters | ||
| ---------- | ||
| coeffs : variables | ||
| Linear coefficients | ||
| intercept : variable, array or integer | ||
| Intercept for linear function (Defaults to zero) | ||
| """ | ||
| Mean.__init__(self) | ||
| Mean.__init__(self, input_dim, active_dims) |
There was a problem hiding this comment.
I think we should use super here.
|
While addressing your corrections I found that Edit: I think this is due to my changes --- subclassing |
| raise ValueError('mean_func must be a subclass of Mean') | ||
| self.M = mean_func | ||
|
|
||
|
|
There was a problem hiding this comment.
Was moving this an aesthetic change, or is there a reason for checking the covariance function first?
There was a problem hiding this comment.
It was so the default mean function could take parameters that the user would have set when they instantiated the covariance function
pymc3/gp/mean.py
Outdated
| over that dimension or column of X. | ||
| """ | ||
|
|
||
| def __init__(self, input_dim, active_dims=None): |
There was a problem hiding this comment.
Is there a downside to having a default of input_dim=1?
There was a problem hiding this comment.
hmm I'm not sure. It's certainly the most likely thing a user would do. When these functions are instantiated they don't know about X yet, so if a user forgets to set it explicitly they'll get an error at runtime.
|
Also, are there other mean functions that might be of interest @fonnesbeck ? |
|
I'm sure there are fancy things you can do with mean functions, but you don't really see anyone doing it. As long as we can apply a little prior information to the mean (which we can with a linear mean) then we are probably set for the time being. |
|
I decided to just address #2078, which was just a simple fix for outputs with the wrong shape. I removed the scope creep... Nother day. I think this should be ready for review then @fonnesbeck @twiecki |
|
These look good. Is this still a WIP, or are you ready to merge? |
|
Yes I think so @fonnesbeck. Those 3 recent commits change the output shape for the mean functions from (n, 1) to (n, ), as per #2262 . |
|
@bwengals Needs a rebase. |
|
@aseyboldt resolved it |
|
The failing tests seem to be due to the numpy update to 1.13 |
|
@fonnesbeck Should we merge this before 3.1? |
|
see #2337 |
|
close in favor of #2337 |
This PR addresses #2078 by fixing the shape of mean function outputs. It also contains tests for the bug in #2078. I had tried including some refactoring but removed this. Now this is just a fix for the mean functions + tests.