Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Inherit 'torch.distributions.exp_family.ExponentialFamily' in MultivariateNormal Class #69430

Closed
wants to merge 14 commits into from
Closed

Inherit 'torch.distributions.exp_family.ExponentialFamily' in MultivariateNormal Class #69430

wants to merge 14 commits into from

Conversation

nonconvexopt
Copy link
Contributor

Fixes #69077
Made a PR to test CI.

@pytorch-probot
Copy link

pytorch-probot bot commented Dec 5, 2021 •
edited
Loading

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/nonconvexopt/pytorch/blob/de0ffa45514f13089fdf8b99acc384128b9b35c6/.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/trunk, ciflow/xla ✅ triggered
linux-docs ciflow/all, ciflow/cpu, ciflow/default, ciflow/docs, ciflow/linux, ciflow/trunk ✅ triggered
linux-vulkan-bionic-py3.6-clang9 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk, ciflow/vulkan ✅ triggered
linux-xenial-cuda11.3-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/default, ciflow/linux, ciflow/trunk ✅ triggered
linux-xenial-cuda11.3-py3.6-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.6-clang7-asan ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/sanitizers, ciflow/trunk ✅ triggered
linux-xenial-py3.6-clang7-onnx ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/onnx, ciflow/trunk ✅ triggered
linux-xenial-py3.6-gcc5.4 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk ✅ triggered
linux-xenial-py3.6-gcc7 ciflow/all, ciflow/cpu, ciflow/default, ciflow/linux, ciflow/trunk ✅ 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
Skipped Workflows
caffe2-linux-xenial-py3.6-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.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/trunk 🚫 skipped
libtorch-linux-xenial-cuda11.3-py3.6-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
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.6-gcc5.4 ciflow/all, ciflow/cpu, ciflow/linux, ciflow/trunk 🚫 skipped
periodic-libtorch-linux-bionic-cuda11.5-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/scheduled 🚫 skipped
periodic-libtorch-linux-xenial-cuda11.1-py3.6-gcc7 ciflow/all, ciflow/cuda, ciflow/libtorch, ciflow/linux, ciflow/scheduled 🚫 skipped
periodic-linux-bionic-cuda11.5-py3.6-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.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
periodic-win-vs2019-cuda11.5-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 Dec 5, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit de0ffa4 (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.

@nonconvexopt
Copy link
Contributor Author

@neerajprad Can you review this PR?

@neerajprad
Copy link
Contributor

neerajprad commented Dec 7, 2021 •
edited
Loading

@nonconvexopt: Sorry for a late reply. From what I understand, the ExponentialFamily inheritance is only useful if we want to automatically get an implementation of the entropy by implementing the abstract methods - _natural_params, _log_normalizer and _mean_carrier_measure. This class already has an explicit entropy method implemented, so there's no advantage to inheriting from ExponentialFamily (we are not implementing the other methods as well). There are other distributions like Binomial which similarly don't inherit from ExponentialFamily for the same reason.

Edit: Outside of that, I suppose there is value in having the natural parameters available to query. If so, we should implement the other methods too if possible and also _from_natural_params method that you will see the other distributions implement.

@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Dec 7, 2021
@nonconvexopt
Copy link
Contributor Author

nonconvexopt commented Dec 8, 2021 •
edited
Loading

Thank you for your detailed explanation. There are not much advantages of inheriting ExponentialFamily. I just thought that since MVN is exponential family distribution, it might be sensible to in herit ExponentialFamily class.

I hope to take a analysis on the codes and try to fit the requirements for ExponentialFamily in this PR later. Please consider it to merge it if it is acceptable.

Thank you for your detailed explanation. I guess there are not much advantages of inheriting ExponentialFamily. I just thought that since multivariate normal is an exponential family distribution, it might be sensible to inherit ExponentialFamily class.

I hope to take a analysis on the codes and try to fit the requirements for ExponentialFamily in this PR later. Please consider it to merge it if it is acceptable.

nonconvexopt and others added 12 commits December 13, 2021 23:16
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
c604e8c 
…variateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
6f64a8a 
…variateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
b2a45b0 
…variateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
efbbfcd 
…variateNormal
@nonconvexopt
Reduced 'math.log' function call and implemented '_log_normalizer' fu…
cb8ef26 
…nction.
@nonconvexopt
git push -u origin
02feffa 
inherit_ExponentialFamily_in_MultivariateNormalMerge branch
'inherit_ExponentialFamily_in_MultivariateNormal' of
github.com:nonconvexopt/pytorch into
inherit_ExponentialFamily_in_MultivariateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
b50d367 
…variateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
31dbbfe 
…variateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
5090c76 
…variateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
4a6d667 
…variateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
b1f27e9 
…variateNormal
@nonconvexopt
Merge branch 'pytorch:master' into inherit_ExponentialFamily_in_Multi…
de0ffa4 
…variateNormal
@nonconvexopt
Copy link
Contributor Author

nonconvexopt commented Dec 20, 2021 •
edited
Loading

Why only the pr/pytorch-linux-bionic-rocm4.3.1-py3.6 keep fails? Not sure.

@nonconvexopt
Copy link
Contributor Author

nonconvexopt commented Dec 21, 2021 •
edited
Loading

@neerajprad This is a trivial change and I think you can merge it if it is beneficial to PyTorch.
Unfortunately, If you don't think so, I will close here and work on other PR.

@nonconvexopt nonconvexopt deleted the inherit_ExponentialFamily_in_MultivariateNormal branch January 10, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neerajprad neerajprad Awaiting requested review from neerajprad

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Why the torch.distributions.MultivariateNormal did not inherited torch.distributions.ExponentialFamily?
5 participants
@nonconvexopt @facebook-github-bot @neerajprad @zou3519 @pytorchbot