-
Notifications
You must be signed in to change notification settings - Fork 249
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 CirculantNormal
distribution.
#1988
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -0,0 +1,293 @@ | |||
{ |
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.
It would be nice to put the legends outside the plots as in https://stackoverflow.com/questions/4700614/how-to-put-the-legend-outside-the-plot :)
Reply via ReviewNB
I think the failing tests are due to issues with the data download and not related to the changes in this PR. There are a few further failing tests due to numerics (maybe associated with a new jax release?). |
5131aa8
to
3a7a1c8
Compare
numpyro/distributions/continuous.py
Outdated
|
||
class CirculantNormal(TransformedDistribution): | ||
""" | ||
Multivariate normal distribution with circulant covariance matrix. |
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.
Could you add reference for this distribution or the circulant covariance matrix?
I don't understand why we call it circulant covariance matrix. The covariance matrix in MVN is symmetric. But the circulant matrix is not symmetric.
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.
I've added a bit more background and a reference. I unfortunately couldn't find a non-paywalled link.
There are still a few failing numerical tests. Is it possible the
|
numpyro/distributions/continuous.py
Outdated
class CirculantNormal(TransformedDistribution): | ||
""" | ||
Multivariate normal distribution with covariance matrix that is positive-definite | ||
and circulant [1], i.e., has periodic boundary conditions. Circulant matrices can be |
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.
I think I understand what is a circulant matrix (from the wikipedia page) but I don't understand why this is a MVN distribution (it seems to be a transformed normal distribution based on the implementation) and why this matrix is called covariance matrix, and what "positive-definite" means here. Could you clarify?
It would be helpful to add formula in the docstrings like https://num.pyro.ai/en/stable/distributions.html#numpyro.distributions.continuous.Levy
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.
The implementation looks great to me! Same for the tutorial. Apart from adding more description to the definition of the distribution, could you also add a thumbnail for it https://github.com/pyro-ppl/numpyro/tree/master/docs/source/_static/img/tutorials
I'm not sure. It seems not robust to multivariate distributions. You can skip it because we have tests for the bijections. |
cac50dc
to
99a566e
Compare
Ah, I forgot the thumbnail. Will add that in a future PR. |
This PR adds a
CirculantNormal
distribution which is a multivariate normal distribution where the covariance matrix has circular boundary conditions. It uses the Fourier transform introduced in #1762 to evaluate the log probability, scaling withn * log(n)
instead of the usualn ** 3
. The PRCirculantNormal
distribution.PackRealFastFourierCoefficientsTransform
transform. This is required to transform a real vector of sizen
to a complex vector of sizen // 2 + 1
which can be passed to therfft
method. See Add complex constraint and real Fourier transform. #1762 (comment) for a brief initial discussion.positive_definite_circulant_vector
constraint. This is really just required for thetest/test_distributions.py::test_distribution_constraints
tests to pass.CirculantNormal
can be used to accelerate Gaussian process inference.norm
parameter).