Debug positive definite constraints #68720
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 67b3448 (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. |
jbschlosser
added
the
triaged
There was a problem hiding this comment.
So, this code assumes that the matrix you're working with is symmetric. Note that the matrix you tried this with is not symmetric. See the definition of [definite matrix]. See also the comment that in the check function. When given a non-symmetric matrix, cholesky copies the lower half of the matrix onto the upper half. In your case, you'd get the matrix [[2, 2], [2, 2]]. This matrix is symmetric positive semidefinite. Due to numerical errors, CPU correctly classifies this as non-SPD and CUDA fails and returns that it's SPD.
Regardless of this, about your solution.
- It's not correct, as a matrix with complex singular values may be considered "positive definite".
- To get the eigenvalues, consider using in the future
linalg.eigvals.
Thank you for detailed explanation! I will check In fact, I have a suggestion in implementation of constraints for matrices: inheritable_torch.distributions.constraints_for_matrices |
There was a problem hiding this comment.
I think that makes sense. Now, I do not maintain this part of PyTorch, so I would not be able to accept such a change in good faith, someone else would need to review it (cc @jbschlosser to find someone that could?).
I have proposed below how to implement that approach.
Co-authored-by: Lezcano <Lezcano@users.noreply.github.com>
Thanks for your feedback. I will open another seperated PR for inheritable_torch.distributions.constraints_for_matrices if it is more clear. |
|
I think @fritzo is our distributions maintainer? |
|
Hi @mruberry I'm pretty busy over the next month, any chance @neerajprad has time to review these PRs? |
Thanks for tagging, I missed this. I'll take a look. |
There are definitely constraints that assume that the matrix corresponding to the event dims is a square matrix so it is good to explicitly check for that. Feel free to update this PR with your proposed change. |
There was a problem hiding this comment.
Looks great overall! I just had one comment about the test. Thanks for contributing to pytorch. It will be good to also get a go-ahead from @lezcano.
…onvexopt/pytorch into debug_positive_definite_constraints
| pytest.param(True, marks=pytest.mark.skipif(not TEST_CUDA, | ||
| reason='CUDA not found.'))]) | ||
| def test_constraint(constraint_fn, result, value, is_cuda): | ||
| t = torch.cuda.DoubleTensor if is_cuda else torch.DoubleTensor |
There was a problem hiding this comment.
I believe that these constructors are deprecated. The correct way of writing this code would be to pass a device to this function and to do torch.tensor(data, dtype=torch.double, device=device).
There was a problem hiding this comment.
I will try to correct it at a new PR since we have to modify all the functions in test/distributions/test_constraints.py. Thanks for your feedback.
|
@neerajprad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…puts. (#69069) Summary: While implementing #68720, We found out empirically that `torch.cholesky_inverse` support batched inputs, but it is not explained in doc: [link](#68720 (review)) `torch.cholesky_inverse` is implemented in #50269 and the doc was updated at #31275 but not merged. neerajprad Pull Request resolved: #69069 Reviewed By: mrshenli Differential Revision: D32979362 Pulled By: neerajprad fbshipit-source-id: 0967c969434ce6e0ab15889c240149c23c0bce44
…puts. (#69069) Summary: While implementing #68720, We found out empirically that `torch.cholesky_inverse` support batched inputs, but it is not explained in doc: [link](#68720 (review)) `torch.cholesky_inverse` is implemented in #50269 and the doc was updated at #31275 but not merged. neerajprad Reviewed By: mrshenli Differential Revision: D32979362 Pulled By: neerajprad fbshipit-source-id: 0967c969434ce6e0ab15889c240149c23c0bce44 [ghstack-poisoned]
…puts. (#69069) Summary: While implementing #68720, We found out empirically that `torch.cholesky_inverse` support batched inputs, but it is not explained in doc: [link](#68720 (review)) `torch.cholesky_inverse` is implemented in #50269 and the doc was updated at #31275 but not merged. neerajprad Reviewed By: mrshenli Differential Revision: D32979362 Pulled By: neerajprad fbshipit-source-id: 0967c969434ce6e0ab15889c240149c23c0bce44
…puts. (#69069) Summary: While implementing #68720, We found out empirically that `torch.cholesky_inverse` support batched inputs, but it is not explained in doc: [link](#68720 (review)) `torch.cholesky_inverse` is implemented in #50269 and the doc was updated at #31275 but not merged. neerajprad Pull Request resolved: #69069 Reviewed By: mrshenli Differential Revision: D32979362 Pulled By: neerajprad fbshipit-source-id: 0967c969434ce6e0ab15889c240149c23c0bce44
…puts. (#69069) Summary: While implementing #68720, We found out empirically that `torch.cholesky_inverse` support batched inputs, but it is not explained in doc: [link](#68720 (review)) `torch.cholesky_inverse` is implemented in #50269 and the doc was updated at #31275 but not merged. neerajprad Pull Request resolved: #69069 Reviewed By: mrshenli Differential Revision: D32979362 Pulled By: neerajprad fbshipit-source-id: 0967c969434ce6e0ab15889c240149c23c0bce44
Summary: Fixes #68050 TODO: - [x] Unit Test - [x] Documentation - [x] Change constraint of matrix variables with 'torch.distributions.constraints.symmetric' if it is reviewed and merged. #68720 Pull Request resolved: #68588 Reviewed By: bdhirsh Differential Revision: D33246843 Pulled By: neerajprad fbshipit-source-id: 825fcddf478555235e7a66de0c18368c41e935cd
Summary: Implement #68050 Reopened merged and reverted PR #68588 worked with neerajprad cc neerajprad Sorry for the confusion. TODO: - [x] Unit Test - [x] Documentation - [x] Change constraint of matrix variables with 'torch.distributions.constraints.symmetric' if it is reviewed and merged. Debug positive definite constraints #68720 Pull Request resolved: #70377 Reviewed By: mikaylagawarecki Differential Revision: D33355132 Pulled By: neerajprad fbshipit-source-id: e968c0d9a3061fb2855564b96074235e46a57b6c
Summary: Implement #68050 Reopened merged and reverted PR #68588 worked with neerajprad cc neerajprad Sorry for the confusion. TODO: - [x] Unit Test - [x] Documentation - [x] Change constraint of matrix variables with 'torch.distributions.constraints.symmetric' if it is reviewed and merged. Debug positive definite constraints #68720 Pull Request resolved: #70377 Reviewed By: mikaylagawarecki Differential Revision: D33355132 Pulled By: neerajprad fbshipit-source-id: e968c0d9a3061fb2855564b96074235e46a57b6c
While implementing #68644,
during the testing of 'torch.distributions.constraint.positive_definite', I found an error in the code: location
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:
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:
By using above implementation, I get following result:
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.