-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add Interpolated distribution class #2163
Conversation
I love the API, not crazy about the name: |
@twiecki The API is dist = Interpolated('interpolated_dist', x_points=x_points, pdf_points=pdf_points) |
Oh, I see. Could we make the |
I doubt that it is possible to come here up with good defaults for The thing I think about is that maybe it is better to name the parameters not Btw it is not necessary to explicitly write parameters names, it might be used as well as just dist = Interpolated('interpolated_dist', x_points, pdf_points) and for example x_points = np.linspace(-10, 10, 100)
pdf_points = np.exp(-x_points*x_points/2) |
Looks good. Could you run the whole notebook? The output of the last cell is not shown. |
@junpenglao Sure! I've updated the notebook. |
I managed to solve the problem of testing by creating temporary subclasses of |
This essentially uses the marginal of the posterior as the prior, right? I.e. we're not taking any possible correlations into account. |
Yes, it is one-dimensional, so it's impossible to take any correlations into account with it. However it is useful when the prior comes not as samples, but for example as a result of some numerical integration, so that the prior is one-dimensional, but complex enough to not be explained in terms of standard distributions. |
super(Interpolated, self).__init__(transform=transform, | ||
*args, **kwargs) | ||
|
||
interp = InterpolatedUnivariateSpline(x_points, pdf_points, k=1, ext='zeros') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow the user to define k
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two major problems with non-linear interpolations:
- PDF can become negative for higher-order spline interpolation at some points, it is nonsensical and would break NUTS sampler. It is also impossible to just replace negative values by zeros because it would make impossible to to use
integral()
method of SciPy spline classes. random()
method can be implemented efficiently only for linear interpolation because inverse CDF can be expressed in closed form only for piecewise-quadratic CDF (well, it is possible to try to do the same for piecewise-cubic CDF using Cardano formula, but I'm not subscribing to it :) For higher-order polynomial interpolations it would be necessary to find inverses numerically, using an iterative process like Newton method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep the first point is a valid concern. I think we can merge this now and add higher order polynomial support in the future.
size=size) | ||
|
||
def logp(self, value): | ||
return tt.log(self.interp_op(value) / self.Z) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I ran locally on your example in #2146 I actually find your first implementation faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was because initially I incorrectly defined the gradient in #2146 , so HMC didn't work well for the second version (see this comment). In this implementation calculation of the gradient is fixed, so it is even faster to put division by the normalization constant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, it should be faster then. I will double check on my side then.
Great work @a-rodin! Thank you for your contribution! |
This PR, as a follow-up on #2146, adds
Interpolated
distribution that takes arbitrary one-dimensional probability density function as an array of values evaluated at some set of points and makes it possible to use this distribution as a prior distribution.The PDF values between the points that are present in the input array are calculated by linear interpolation, gradient and random sampling are defined accordingly.
Time complexity of evaluation is
O(log n)
wheren
is the number of points in the input array. The step size doesn't have to be the same at all points, so it is possible to make it smaller for the regions of measure concentration and larger for other regions.I implemented some tests for
random
function, but unfortunately I was not able to do the same forlogp
function (but added a test template topymc3/tests/test_distributions.py
). The reason is that this distribution takes plain numpy arrays instead of variables as inputs, because anyway the dimensionality of the input vectors is supposed to be large and it would not be feasible to sample them, but the test suite is supposed to work with distributions that take variables as inputs.