-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Improve numerical stability of torch.distributions.wishart.Wishart
#72993
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
Improve numerical stability of torch.distributions.wishart.Wishart
#72993
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
|
|
In the Wikepedia Exponential Family page, natural parameter of Wishart distribution is noted as: For optimization, I tried to transform |
|
@neerajprad @vadimkantorov @fritzo I think I am not allowed to reproduce the macos errors. Does the waiting for the further maintenance of CI will solve the issue? |
fritzo
left a comment
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.
LGTM. This is fine, but imho x / 2 is easier to read than 0.5 * x everywhere. I believe test failures are unrelated (pip failures, onnx registration errors).
…convexopt/pytorch into distributions/wishart_improvement_v1
|
I think it is ready to be merged. |
fritzo
left a comment
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.
LGTM.
Test errors appear unrelated:
Details
botocore.exceptions.ClientError: An error occurred (AccessDeniedException) when calling the Invoke operation: User: arn:aws:iam::308535385114:user/pytorch-pytorch-ossci-s3-read-write is not authorized to perform: lambda:InvokeFunction on resource: arn:aws:lambda:us-east-1:308535385114:function:rds-proxy because no identity-based policy allows the lambda:InvokeFunction action
@neerajprad would you be able to merge this?
|
Added trivial change: fix wrong parameter input (df with |
|
@pytorchbot ciflow rerun |
|
This command didn't do anything. |
|
LGTM |
fritzo
left a comment
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.
LGTM @neerajprad I believe this is ready to merge
|
@neerajprad has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Thank you for the richful feedbacks and reviews. I appreciated your kindness. I am considering implementing Multivariate T distribution for PyTorch if there is no one trying after I finish Inverse Wishart #70275. It's a shame that I am delaying it to this far. I guess some people are trying to use T Process similary to Gaussian Process these days. It seems that probabilistic ML libraries are actively implementing it. It might be great if PyTorch offers official implementation for Multivariate T used for T process. |
|
@nonconvexopt it's a pleasure to work with you. Yes Multivariate Student't t might be useful for T processes, but I'm not sure: IIUC it is just a Gamma-MultivariateNormal chain where the gamma rv can be analytically integrated out, and I believe in T processes there are local gammas, but in mutlivariate T there is a single gamma? Let's move this discussion to a new issue. Also cc @fehiepsi who worked out a lot of that math in Pyro. |
|
There is MVT distribution in Pyro, but it only supports |
…72993) Summary: Maintanance of #70377 Multiple modifications of the merged initial implementation of Wishart distribution. Key modifications: * torch/distributions/wishart.py: Clamp (Clip) float type values to calculate reciprocal in numerically stable manner, by using the eps value paired to each torch.dtype * test/distributions/test_distributions.py: Test Wishart distribution implementation in numerically unstable zones, i.e df values are at ndim - 1 < df < ndim where ndim is the one dimenstion of the Wishart parameter & sample matrix. Re-opened reverted PR #72059 cc neerajprad vadimkantorov Pull Request resolved: #72993 Reviewed By: samdow Differential Revision: D34853807 Pulled By: neerajprad fbshipit-source-id: eb62dca19bf8a934fdf59b4ffc58587447fe8378
Maintanance of #70377
Multiple modifications of the merged initial implementation of Wishart distribution.
Key modifications:
Re-opened reverted PR #72059
cc @neerajprad @vadimkantorov