Skip to content

Conversation

@nonconvexopt
Copy link
Contributor

@nonconvexopt nonconvexopt commented Nov 19, 2021

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

@pytorch-probot
Copy link

pytorch-probot bot commented Nov 19, 2021

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/nonconvexopt/pytorch/blob/510f7b46ebab2df8b38db93440f0002bbb3cf473/.github/generated-ciflow-ruleset.json
PR ciflow labels: ciflow/default

Workflows Labels (bold enabled) Status
Triggered Workflows
linux-bionic-py3.6-clang9 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/noarch, ciflow/xla ✅ triggered
linux-vulkan-bionic-py3.6-clang9 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/vulkan ✅ triggered
linux-xenial-cuda11.3-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/default, ciflow/linux ✅ triggered
linux-xenial-py3-clang5-mobile-build ciflow/all, ciflow/default, ciflow/linux, ciflow/mobile ✅ triggered
linux-xenial-py3-clang5-mobile-custom-build-static ciflow/all, ciflow/default, ciflow/linux, ciflow/mobile ✅ triggered
linux-xenial-py3.6-clang7-asan ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/sanitizers ✅ triggered
linux-xenial-py3.6-clang7-onnx ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/onnx ✅ triggered
linux-xenial-py3.6-gcc5.4 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux ✅ triggered
linux-xenial-py3.6-gcc7 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux ✅ triggered
linux-xenial-py3.6-gcc7-bazel-test ciflow/all, ciflow/bazel, ciflow/cpu, ciflow/default, ciflow/linux ✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single ciflow/all, ciflow/android, ciflow/cpu, ciflow/default, ciflow/linux ✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single-full-jit ciflow/all, ciflow/android, ciflow/cpu, ciflow/default, ciflow/linux ✅ triggered
win-vs2019-cpu-py3 ciflow/all, ciflow/cpu, ciflow/default, ciflow/win ✅ triggered
win-vs2019-cuda11.3-py3 ciflow/all, ciflow/cuda, ciflow/default, ciflow/win ✅ triggered
Skipped Workflows
caffe2-linux-xenial-py3.6-gcc5.4 ciflow/all, ciflow/cpu, ciflow/linux 🚫 skipped
docker-builds ciflow/all 🚫 skipped
ios-12-5-1-arm64 ciflow/all, ciflow/ios, ciflow/macos 🚫 skipped
ios-12-5-1-arm64-coreml ciflow/all, ciflow/ios, ciflow/macos 🚫 skipped
ios-12-5-1-arm64-custom-ops ciflow/all, ciflow/ios, ciflow/macos 🚫 skipped
ios-12-5-1-arm64-full-jit ciflow/all, ciflow/ios, ciflow/macos 🚫 skipped
ios-12-5-1-arm64-metal ciflow/all, ciflow/ios, ciflow/macos 🚫 skipped
ios-12-5-1-x86-64 ciflow/all, ciflow/ios, ciflow/macos 🚫 skipped
ios-12-5-1-x86-64-coreml ciflow/all, ciflow/ios, ciflow/macos 🚫 skipped
ios-12-5-1-x86-64-full-jit ciflow/all, ciflow/ios, ciflow/macos 🚫 skipped
libtorch-linux-xenial-cuda10.2-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux 🚫 skipped
libtorch-linux-xenial-cuda11.3-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux 🚫 skipped
linux-bionic-cuda10.2-py3.9-gcc7 ciflow/all, ciflow/cuda, ciflow/linux, ciflow/slow 🚫 skipped
macos-10-15-py3-arm64 ciflow/all, ciflow/macos 🚫 skipped
macos-10-15-py3-lite-interpreter-x86-64 ciflow/all, ciflow/macos 🚫 skipped
macos-10-15-py3-x86-64 ciflow/all, ciflow/macos 🚫 skipped
parallelnative-linux-xenial-py3.6-gcc5.4 ciflow/all, ciflow/cpu, ciflow/linux 🚫 skipped
periodic-libtorch-linux-xenial-cuda11.1-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/scheduled 🚫 skipped
periodic-linux-xenial-cuda10.2-py3-gcc7-slow-gradcheck ciflow/all, ciflow/cuda, ciflow/linux, ciflow/scheduled, ciflow/slow, ciflow/slow-gradcheck 🚫 skipped
periodic-linux-xenial-cuda11.1-py3.6-gcc7-debug ciflow/all, ciflow/cuda, ciflow/linux, ciflow/scheduled 🚫 skipped
periodic-win-vs2019-cuda11.1-py3 ciflow/all, ciflow/cuda, ciflow/scheduled, ciflow/win 🚫 skipped

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/slow

For more information, please take a look at the CI Flow Wiki.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 19, 2021

🔗 Helpful links

💊 CI failures summary and remediations

As 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.

Click here to manually regenerate this comment.

@dagitses dagitses requested a review from ejguan November 19, 2021 15:41
@dagitses dagitses added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 19, 2021
@ejguan ejguan requested review from fritzo and removed request for ejguan November 19, 2021 15:42
@ejguan ejguan removed their assignment Nov 19, 2021
@neerajprad
Copy link
Contributor

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 positive_definite so that matrices like [[2., 0.], [2., 2.]] will fail this constraint but pass the positive definite constraint?

@nonconvexopt
Copy link
Contributor Author

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 positive_definite so that matrices like [[2., 0.], [2., 2.]] will fail this constraint but pass the positive definite constraint?

Sure, it is my pleasure. Thank you for giving me detailed advice. I will test it out soon!

@nonconvexopt
Copy link
Contributor Author

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 positive_definite so that matrices like [[2., 0.], [2., 2.]] will fail this constraint but pass the positive definite constraint?

@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.

@nonconvexopt
Copy link
Contributor Author

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.
Also, I found an error in the implementation of 'torch.distributions.constraints.positive_definite' which I will make another PR.

@neerajprad neerajprad added the module: distributions Related to torch.distributions label Nov 22, 2021
@neerajprad
Copy link
Contributor

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])
Copy link
Contributor

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)?

Copy link
Contributor Author

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!

@facebook-github-bot
Copy link
Contributor

@neerajprad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@neerajprad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@neerajprad merged this pull request in bc3bdbc.

@nonconvexopt
Copy link
Contributor Author

nonconvexopt commented Nov 24, 2021

CI errors are caused due to the tests about constraints.positive_definite:
(constraints.positive_definite, True, [[2., 0], [2., 2]]),
This is the friendly reminder, explanation from Lezcano: Debug positive definite constraints
We can remove the test to pass the CI checks.

@neerajprad
Copy link
Contributor

This is the friendly reminder, explanation from Lezcano: Debug positive definite constraints
We can remove the test to pass the CI checks.

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 pytest.xfail) if this is expected to fail with CUDA.

@ngimel
Copy link
Collaborator

ngimel commented Nov 25, 2021

Breaks cuda 10.2 build, reverting, see e.g. https://github.com/pytorch/pytorch/runs/4318116960?check_suite_focus=true

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by f14c16e. To re-land this change, follow these steps.

nonconvexopt added a commit to nonconvexopt/pytorch that referenced this pull request Nov 28, 2021
PaliC added a commit that referenced this pull request Nov 30, 2021
…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]
facebook-github-bot pushed a commit that referenced this pull request Dec 1, 2021
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
@malfet malfet mentioned this pull request Dec 7, 2021
@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by f14c16e. To re-land this change, follow these steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged module: distributions Related to torch.distributions open source Reverted triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants