Skip to content

Conversation

@nonconvexopt
Copy link
Contributor

@nonconvexopt nonconvexopt commented Feb 17, 2022

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

@pytorch-bot
Copy link

pytorch-bot bot commented Feb 17, 2022

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/nonconvexopt/pytorch/blob/1a2703c358cb97de0cb567bbf5a0fc704fee7bc4/.github/generated-ciflow-ruleset.json
PR ciflow labels: ciflow/default
Add ciflow labels to this PR to trigger more builds:

Workflows Labels (bold enabled) Status
Triggered Workflows
linux-binary-conda ciflow/binaries, ciflow/binaries_conda, ciflow/default ✅ triggered
linux-binary-libtorch-cxx11-abi ciflow/binaries, ciflow/binaries_libtorch, ciflow/default ✅ triggered
linux-binary-libtorch-pre-cxx11 ciflow/binaries, ciflow/binaries_libtorch, ciflow/default ✅ triggered
linux-binary-manywheel ciflow/binaries, ciflow/binaries_wheel, ciflow/default ✅ triggered
linux-bionic-py3.7-clang9 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/noarch, ciflow/trunk ✅ triggered
linux-bionic-rocm4.5-py3.7 ciflow/all, ciflow/default, ciflow/linux, ciflow/rocm, ciflow/trunk ✅ triggered
linux-docs ciflow/all, ciflow/cpu, ciflow/default, ciflow/docs, ciflow/linux, ciflow/trunk ✅ triggered
linux-vulkan-bionic-py3.7-clang9 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk, ciflow/vulkan ✅ triggered
linux-xenial-cuda11.3-py3.7-gcc7 ciflow/all, ciflow/cuda, ciflow/default, ciflow/linux, ciflow/trunk ✅ triggered
linux-xenial-cuda11.3-py3.7-gcc7-bazel-test ciflow/all, ciflow/bazel, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk ✅ triggered
linux-xenial-py3-clang5-mobile-build ciflow/all, ciflow/default, ciflow/linux, ciflow/mobile, ciflow/trunk ✅ triggered
linux-xenial-py3-clang5-mobile-custom-build-static ciflow/all, ciflow/default, ciflow/linux, ciflow/mobile, ciflow/trunk ✅ triggered
linux-xenial-py3.7-clang7-asan ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/sanitizers, ciflow/trunk ✅ triggered
linux-xenial-py3.7-clang7-onnx ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/onnx, ciflow/trunk ✅ triggered
linux-xenial-py3.7-gcc5.4 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk ✅ triggered
linux-xenial-py3.7-gcc7 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk ✅ triggered
linux-xenial-py3.7-gcc7-no-ops ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk ✅ triggered
macos-arm64-binary-conda ciflow/binaries, ciflow/binaries_conda, ciflow/default ✅ triggered
macos-arm64-binary-wheel ciflow/binaries, ciflow/binaries_wheel, ciflow/default ✅ triggered
macos-binary-conda ciflow/binaries, ciflow/binaries_conda, ciflow/default ✅ triggered
macos-binary-libtorch-cxx11-abi ciflow/binaries, ciflow/binaries_libtorch, ciflow/default ✅ triggered
macos-binary-libtorch-pre-cxx11 ciflow/binaries, ciflow/binaries_libtorch, ciflow/default ✅ triggered
macos-binary-wheel ciflow/binaries, ciflow/binaries_wheel, ciflow/default ✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single ciflow/all, ciflow/android, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk ✅ 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, ciflow/trunk ✅ triggered
win-vs2019-cpu-py3 ciflow/all, ciflow/cpu, ciflow/default, ciflow/trunk, ciflow/win ✅ triggered
win-vs2019-cuda11.3-py3 ciflow/all, ciflow/cuda, ciflow/default, ciflow/trunk, ciflow/win ✅ triggered
windows-binary-libtorch-cxx11-abi ciflow/binaries, ciflow/binaries_libtorch, ciflow/default ✅ triggered
windows-binary-libtorch-pre-cxx11 ciflow/binaries, ciflow/binaries_libtorch, ciflow/default ✅ triggered
windows-binary-wheel ciflow/binaries, ciflow/binaries_wheel, ciflow/default ✅ triggered
Skipped Workflows
caffe2-linux-xenial-py3.7-gcc5.4 ciflow/all, ciflow/cpu, ciflow/linux, ciflow/trunk 🚫 skipped
docker-builds ciflow/all, ciflow/trunk 🚫 skipped
ios-12-5-1-arm64 ciflow/all, ciflow/ios, ciflow/macos, ciflow/trunk 🚫 skipped
ios-12-5-1-arm64-coreml ciflow/all, ciflow/ios, ciflow/macos, ciflow/trunk 🚫 skipped
ios-12-5-1-arm64-custom-ops ciflow/all, ciflow/ios, ciflow/macos, ciflow/trunk 🚫 skipped
ios-12-5-1-arm64-full-jit ciflow/all, ciflow/ios, ciflow/macos, ciflow/trunk 🚫 skipped
ios-12-5-1-arm64-metal ciflow/all, ciflow/ios, ciflow/macos, ciflow/trunk 🚫 skipped
ios-12-5-1-x86-64 ciflow/all, ciflow/ios, ciflow/macos, ciflow/trunk 🚫 skipped
ios-12-5-1-x86-64-coreml ciflow/all, ciflow/ios, ciflow/macos, ciflow/trunk 🚫 skipped
ios-12-5-1-x86-64-full-jit ciflow/all, ciflow/ios, ciflow/macos, ciflow/trunk 🚫 skipped
libtorch-linux-xenial-cuda10.2-py3.7-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/trunk 🚫 skipped
libtorch-linux-xenial-cuda11.3-py3.7-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/trunk 🚫 skipped
linux-bionic-cuda10.2-py3.9-gcc7 ciflow/all, ciflow/cuda, ciflow/linux, ciflow/slow, ciflow/trunk 🚫 skipped
linux-docs-push ciflow/all, ciflow/cpu, ciflow/linux, ciflow/scheduled 🚫 skipped
linux-xenial-cuda11.3-py3.7-gcc7-no-ops ciflow/all, ciflow/cuda, ciflow/linux, ciflow/trunk 🚫 skipped
macos-10-15-py3-arm64 ciflow/all, ciflow/macos, ciflow/trunk 🚫 skipped
macos-10-15-py3-lite-interpreter-x86-64 ciflow/all, ciflow/macos, ciflow/trunk 🚫 skipped
macos-11-py3-x86-64 ciflow/all, ciflow/macos, ciflow/trunk 🚫 skipped
parallelnative-linux-xenial-py3.7-gcc5.4 ciflow/all, ciflow/cpu, ciflow/linux, ciflow/trunk 🚫 skipped
periodic-libtorch-linux-bionic-cuda11.5-py3.7-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/scheduled 🚫 skipped
periodic-libtorch-linux-xenial-cuda11.1-py3.7-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/scheduled 🚫 skipped
periodic-linux-bionic-cuda11.5-py3.7-gcc7 ciflow/all, ciflow/cuda, 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.7-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
periodic-win-vs2019-cuda11.5-py3 ciflow/all, ciflow/cuda, ciflow/scheduled, ciflow/win 🚫 skipped
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-build ciflow/all, ciflow/android, ciflow/cpu, ciflow/linux, ciflow/trunk 🚫 skipped
pytorch-xla-linux-bionic-py3.7-clang8 ciflow/all, ciflow/cpu, ciflow/linux, ciflow/trunk, ciflow/xla 🚫 skipped

@nonconvexopt
Copy link
Contributor Author

nonconvexopt commented Feb 25, 2022

In the Wikepedia Exponential Family page, natural parameter of Wishart distribution is noted as: -1/2 * V^-1 and (n-p-1)/2 based on most widely used representation of WIshart distribution.

For optimization, I tried to transform (n-p-1)/2 to n/2. Mechanism of exponential familiy entropy calculation implemented at torch.distributions.exp_family.ExponentialFamily calculates entropy by the dot product of natural parameters and the derivative of log probability with respect to natural parameters. It does not yield correct entropy when natural parameters are transformed. Thus, I will revert the changes for now.

@nonconvexopt
Copy link
Contributor Author

nonconvexopt commented Feb 25, 2022

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

test (default, 1, 2, macos-11)

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
Error: Process completed with exit code 1.

Copy link
Collaborator

@fritzo fritzo left a 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).

@nonconvexopt
Copy link
Contributor Author

I think it is ready to be merged.

Copy link
Collaborator

@fritzo fritzo left a 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?

@nonconvexopt
Copy link
Contributor Author

nonconvexopt commented Mar 7, 2022

Added trivial change: fix wrong parameter input (df with ndim - 1 < df < ndim) in Scipy Wishart class.
macos test failure was due to my mistake.

@nonconvexopt
Copy link
Contributor Author

@pytorchbot ciflow rerun

@pytorch-bot
Copy link

pytorch-bot bot commented Mar 7, 2022

This command didn't do anything.
You don't need to manually issue ciflow rerun commands anymore. Just adding a ciflow/ label will trigger the workflow.

@nonconvexopt
Copy link
Contributor Author

LGTM

Copy link
Collaborator

@fritzo fritzo left a 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

@facebook-github-bot
Copy link
Contributor

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

@nonconvexopt
Copy link
Contributor Author

nonconvexopt commented Mar 15, 2022

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.

@fritzo
Copy link
Collaborator

fritzo commented Mar 15, 2022

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

@fehiepsi
Copy link
Contributor

There is MVT distribution in Pyro, but it only supports scale_tril input. If we port/reimplement in PyTorch, I think we need to support covariance_matrix, precision_matrix inputs too.

facebook-github-bot pushed a commit that referenced this pull request Mar 15, 2022
…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
@nonconvexopt nonconvexopt deleted the distributions/wishart_improvement_v1 branch March 23, 2022 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed open source 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.

8 participants