-
Notifications
You must be signed in to change notification settings - Fork 26.3k
implemented 'torch.distributions.constraints.symmetric' checking if the tensor is symmetric at last 2 dimension. #68644
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
Conversation
…sor is symmetric in last 2 dimensions.
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 510f7b4 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
Looks great. Could you add as small test to test_constraints which tests this on a few matrices and verifies that this can be stacked with |
Sure, it is my pleasure. Thank you for giving me detailed advice. I will test it out soon! |
@neerajprad How about this design? inheritable_torch.distributions.constraints_for_matrices It is different from previous implementation styles in 'torch.distributions.constraints' and it requires more computation. But more logical way. |
|
This is the best way I think. I followed the design styles from 'test/distributions/test_distributions.py' which used 'unittest' package rather than 'pytest' package. |
|
Looks good to me! Thanks for contributing, I'll review your PR for the Wishart distribution after this. |
| t = torch.cuda.DoubleTensor if is_cuda else torch.DoubleTensor | ||
| return constraint_fn(*(t(x) if isinstance(x, list) else x for x in args)) | ||
|
|
||
| @pytest.mark.parametrize('constraint_fn, result, value', [(e[0], e[1], e[2]) for e in EXAMPLES]) |
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.
nit: why not just @pytest.mark.parametrize('constraint_fn, result, value', EXAMPLES)?
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. Thanks for checking!
|
@neerajprad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@neerajprad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@neerajprad merged this pull request in bc3bdbc. |
|
CI errors are caused due to the tests about constraints.positive_definite: |
I didn't see any failures related to this. Could you point me to the test errors, are the failures deterministic? We should definitely skip the test (or add |
|
Breaks cuda 10.2 build, reverting, see e.g. https://github.com/pytorch/pytorch/runs/4318116960?check_suite_focus=true |
|
This pull request has been reverted by f14c16e. To re-land this change, follow these steps. |
…he tensor is symmetric at last 2 dimension. (#68644) Summary: Implemented submodule for #68050 Opened cleaned, final version of PR for #68240 Explanation: I am trying to contribute to PyTorch by implementing distributions for symmetric matrices like Wishart distribution and Inverse Wishart distribution. Although there is a LKJ distribution for the Cholesky decomposition of correlation matrices, it only represents equivalence to restricted form of Wishart distribution. [https://arxiv.org/abs/1809.04746](https://arxiv.org/abs/1809.04746) Thus, I started implementing Wishart distribution and Inverse Wishart distribution seperately. I added a short code about the 'torch.distributions.constraints.symmetric', which was not included in 'torch.distributions.constraints' previously. i.e., 'torch.distributions.constraints' contains module like 'positive_definite' constraints, but it just assumes symmetricity of the input matrix. [Link](https://github.com/pytorch/pytorch/blob/1adeeabdc0c8832420c091c5c668843768530d7f/torch/distributions/constraints.py#L466) So, I think it will be better if we have constraint checking symmetricity of the tensors in PyTorch. We may further utilize it like `constraints.stack([constraints.symmetric, constraints.positive_definite])` for the constraint of the covariance matrix in Multivariate Normal distribution, for example, to check if the random matrix is a symmetric positive definite matrix. cc fritzo neerajprad alicanb nikitaved Reviewed By: jbschlosser Differential Revision: D32599540 Pulled By: neerajprad fbshipit-source-id: 9227f7e9931834a548a88da69e4f2e9af7732cfe [ghstack-poisoned]
Summary: While implementing #68644, during the testing of 'torch.distributions.constraint.positive_definite', I found an error in the code: [location](https://github.com/pytorch/pytorch/blob/c7ecf1498d961415006c3710ac8d99166fe5d634/torch/distributions/constraints.py#L465-L468) ``` class _PositiveDefinite(Constraint): """ Constrain to positive-definite matrices. """ event_dim = 2 def check(self, value): # Assumes that the matrix or batch of matrices in value are symmetric # info == 0 means no error, that is, it's SPD return torch.linalg.cholesky_ex(value).info.eq(0).unsqueeze(0) ``` The error is caused when I check the positive definiteness of `torch.cuda.DoubleTensor([[2., 0], [2., 2]])` But it did not made a problem for `torch.DoubleTensor([[2., 0], [2., 2]])` You may easily reproduce the error by following code: ``` Python 3.9.7 (default, Sep 16 2021, 13:09:58) [GCC 7.5.0] :: Anaconda, Inc. on linux Type "help", "copyright", "credits" or "license" for more information. >>> import torch >>> const = torch.distributions.constraints.positive_definite >>> const.check(torch.cuda.DoubleTensor([[2., 0], [2., 2]])) tensor([False], device='cuda:0') >>> const.check(torch.DoubleTensor([[2., 0], [2., 2]])) tensor([True]) ``` The cause of error can be analyzed more if you give 'check_errors = True' as a additional argument for 'torch.linalg.cholesky_ex'. It seem that it is caused by the recent changes in 'torch.linalg'. And, I suggest to modify the '_PositiveDefinite' class by using 'torch.linalg.eig' function like the below: ``` class _PositiveDefinite(Constraint): """ Constrain to positive-definite matrices. """ event_dim = 2 def check(self, value): return (torch.linalg.eig(value)[0].real > 0).all(dim=-1) ``` By using above implementation, I get following result: ``` Python 3.9.7 (default, Sep 16 2021, 13:09:58) [GCC 7.5.0] :: Anaconda, Inc. on linux Type "help", "copyright", "credits" or "license" for more information. >>> import torch >>> const = torch.distributions.constraints.positive_definite >>> const.check(torch.cuda.DoubleTensor([[2., 0.], [2., 2.]])) tensor(True, device='cuda:0') >>> const.check(torch.DoubleTensor([[2., 0.], [2., 2.]])) tensor(True) ``` FYI, I do not know what algorithm is used in 'torch.linalg.eig' and 'torch.linalg.cholesky_ex'. As far as I know, they have same time complexity generally, O(n^3). It seems that in case you used special algorithms or finer parallelization, time complexity of Cholesky decomposition may be reduced to approximately O(n^2.5). If there is a reason 'torch.distributions.constraints.positive_definite' used 'torch.linalg.cholesky_ex' rather than 'torch.linalg.eig' previously, I hope to know. Pull Request resolved: #68720 Reviewed By: samdow Differential Revision: D32724391 Pulled By: neerajprad fbshipit-source-id: 32e2a04b2d5b5ddf57a3de50f995131d279ede49
|
This pull request has been reverted by f14c16e. To re-land this change, follow these steps. |
Implemented submodule for #68050
Opened cleaned, final version of PR for #68240
Explanation:
I am trying to contribute to PyTorch by implementing distributions for symmetric matrices like Wishart distribution and Inverse Wishart distribution. Although there is a LKJ distribution for the Cholesky decomposition of correlation matrices, it only represents equivalence to restricted form of Wishart distribution. https://arxiv.org/abs/1809.04746 Thus, I started implementing Wishart distribution and Inverse Wishart distribution seperately.
I added a short code about the 'torch.distributions.constraints.symmetric', which was not included in 'torch.distributions.constraints' previously. i.e., 'torch.distributions.constraints' contains module like 'positive_definite' constraints, but it just assumes symmetricity of the input matrix. Link So, I think it will be better if we have constraint checking symmetricity of the tensors in PyTorch.
We may further utilize it like
constraints.stack([constraints.symmetric, constraints.positive_definite])for the constraint of the covariance matrix in Multivariate Normal distribution, for example, to check if the random matrix is a symmetric positive definite matrix.
cc @fritzo @neerajprad @alicanb @nikitaved