Add a transform for positive-definite matrices. #76777

tillahoffmann · 2022-05-03T23:59:22Z

The PositiveDefiniteTransform is required to transform from an unconstrained space to positive definite matrices, e.g. to support testing the Wishart mode in #76690. It is a simple extension of the LowerCholeskyTransform.

I've also added a small test that ensures the generated data belong to the domain of the associated transform. Previously, the data generated for the inverse transform of the LowerCholeskyTransform wasn't part of the domain, and the test only passed because the comparison uses equal_nan=True.

cc @fritzo, @neerajprad

facebook-github-bot · 2022-05-03T23:59:28Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/76777
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

As of commit c61fde6e3f (more details on the Dr. CI page):

Expand to see more

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

lezcano

The implementation makes sense to me. I'll let others review whether this is a reasonable PR in the context of distributions (I am not involved in its maintenance).

torch/distributions/transforms.py

neerajprad · 2022-05-08T00:51:26Z

LGTM, thanks for adding this! cc @nonconvexopt - could you take a look as well?

torch/distributions/transforms.py

nonconvexopt · 2022-05-08T04:35:22Z

LGTM, thanks for adding this! cc @nonconvexopt - could you take a look as well?

@neerajprad Thank you for tagging me. It is good to know how PyTorch is improving.
@tillahoffmann I think it is essential implementation to the torch.distributions for future transformed distribution developement. Thank you for the contribution.

tillahoffmann · 2022-07-01T15:30:37Z

I think this one is ready to be merged. Would you be able to take another look, @neerajprad? Thank you for the time reviewing my distribution-related PRs.

facebook-github-bot · 2022-10-03T23:02:52Z

/easycla

As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details.

This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign.

linux-foundation-easycla · 2022-10-03T23:02:56Z

The committers listed above are authorized under a signed CLA.

✅ login: tillahoffmann / name: Till Hoffmann (395a36d044655b6c19b32b264a3aa2c438774d28, 519a419f9c9cad1672a5f98917abaa787f8c24ce, 61430a6fc650bf5ebfbe69f7cbe4fa29cd642413, 3027c02a8f497cdd7b51b3c33bce4f1699194384, c61fde6e3f8aee85ea30a1a5804488ee029b3eb0)

tillahoffmann · 2022-11-30T16:04:21Z

Hi @neerajprad, I've now signed the CLA. Would you mind taking another look?

lezcano · 2022-11-30T16:15:20Z

@pytorchbot rebase

lezcano

The linalg part looks good. I'll let the people from distributions review the rest.

pytorchmergebot · 2022-11-30T16:17:28Z

@pytorchbot successfully started a rebase job. Check the current status here

pytorchmergebot · 2022-11-30T16:17:35Z

Successfully rebased positivedefinite onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout positivedefinite && git pull --rebase)

pytorch-bot · 2022-11-30T16:17:43Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/76777

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 378a0cd:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

lezcano · 2022-11-30T22:26:27Z

@fritzo, @neerajprad PTAL

fritzo

Thanks @tillahoffmann, everything looks good except my one comment on inheritance. After that's fixed LGTM!

tillahoffmann · 2022-12-02T16:02:42Z

Thanks for the review, @fritzo. I couldn't find the comment on inheritance, unfortunately. Which line was that on?

fritzo · 2022-12-02T18:56:31Z

torch/distributions/transforms.py

Please avoid inheriting from LowerCholeskyTransform here. Instead either own an instance or use the global instance. I think something like this should work:

class PositiveDefiniteTransform(Transform): def __init__(self, cache_size=0): super.__init__(cache_size=cache_size) self._lower_cholesky = LowerCholeskyTransform(cache_size=cache_size) def _call(self, x): x = self._lower_cholesky(x) return x @ x.mT def _inverse(self, y): y = torch.linalg.cholesky(y) return self._lower_cholesky.inv(y)

just for my understanding, why is this prefered over the current approach?

In practice, downstream users may dispatch on transforms via singledispatch, multipledispatch, or isinstance, which could yield incorrect behavior due to the false match. This could also lead to a missed typechecking error, where a LowerCholeskyTransform is required, a PositiveDefiniteTransform is provided, and the typechecker erroneously thinks that's fine.

In theory, the two classes do not satisfy a subtyping relation, an is-a relationship: PositiveDefiniteTransform is not a special case of LowerCholeskyTransform.

(sorry I had tried to explain in my earlier review, but apparently forgot to save the comment)

I've removed the inheritance. Overriding __init__ also required overriding with_cache (see below), and I decided to instantiate a new LowerCholeskyTransform for each transform. The overhead seems minimal, but happy to make adjustments as you see fit. I couldn't find the global instance of LowerCholeskyTransform.

pytorch/torch/distributions/transforms.py

Lines 135 to 137 in 777ac63

if type(self).__init__ is Transform.__init__:

return type(self)(cache_size=cache_size)

raise NotImplementedError("{}.with_cache is not implemented".format(type(self)))

fritzo

LGTM, thanks for resolving the inheritance issue!

fritzo · 2022-12-05T15:36:03Z

@lezcano would you merge this please? (i believe i lost merge privileges)

lezcano · 2022-12-05T15:44:22Z

@pytorchbot merge

pytorchmergebot · 2022-12-05T15:47:17Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2022-12-05T16:37:48Z

Merge failed

Reason: The following mandatory check(s) failed (Rule Distributions):

pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

soumith · 2022-12-08T06:15:42Z

@pytorchbot rebase

pytorchmergebot · 2022-12-08T06:17:29Z

@pytorchbot successfully started a rebase job. Check the current status here

Co-authored-by: Mario Lezcano Casado <3291265+lezcano@users.noreply.github.com>

pytorchmergebot · 2022-12-08T06:17:33Z

Successfully rebased positivedefinite onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout positivedefinite && git pull --rebase)

soumith · 2022-12-08T06:19:39Z

@pytorchbot merge

pytorchmergebot · 2022-12-08T06:21:50Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

The `PositiveDefiniteTransform` is required to transform from an unconstrained space to positive definite matrices, e.g. to support testing the Wishart mode in pytorch#76690. It is a simple extension of the `LowerCholeskyTransform`. I've also added a small test that ensures the generated data belong to the domain of the associated transform. Previously, the data generated for the inverse transform of the `LowerCholeskyTransform` wasn't part of the domain, and the test only passed because the comparison uses `equal_nan=True`. Pull Request resolved: pytorch#76777 Approved by: https://github.com/lezcano, https://github.com/fritzo, https://github.com/soumith

facebook-github-bot added the cla signed label May 3, 2022

pytorchbot added the open source label May 4, 2022

tillahoffmann mentioned this pull request May 4, 2022

Add mode property to distributions. #76690

Closed

bdhirsh requested review from lezcano and neerajprad May 4, 2022 21:02

bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 4, 2022

lezcano reviewed May 5, 2022

View reviewed changes

torch/distributions/transforms.py Outdated Show resolved Hide resolved

nonconvexopt reviewed May 8, 2022

View reviewed changes

torch/distributions/transforms.py Outdated Show resolved Hide resolved

tillahoffmann force-pushed the positivedefinite branch from 59b363f to 26e98be Compare May 11, 2022 19:03

tillahoffmann force-pushed the positivedefinite branch from 26e98be to efb3359 Compare July 26, 2022 02:25

tillahoffmann force-pushed the positivedefinite branch from efb3359 to c61fde6 Compare August 13, 2022 17:24

lezcano approved these changes Nov 30, 2022

View reviewed changes

pytorchmergebot force-pushed the positivedefinite branch from c61fde6 to 87023fa Compare November 30, 2022 16:17

fritzo reviewed Dec 2, 2022

View reviewed changes

tillahoffmann force-pushed the positivedefinite branch from c91d458 to 7e8d1df Compare December 5, 2022 14:56

fritzo approved these changes Dec 5, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Dec 5, 2022

soumith approved these changes Dec 8, 2022

View reviewed changes

tillahoffmann and others added 7 commits December 8, 2022 06:17

Ensure generated data belongs to the domain.

8760b47

Add PositiveDefiniteTransform.

0fdad4f

Ignore type check for codomain of PositiveDefiniteTransform.

1aa79ef

Add error code to type ignore in PositiveDefiniteTransform.

928bc38

Add Wishart distribution to mode tests.

8362f5e

Use matrix transpose mT.

2041aa7

Co-authored-by: Mario Lezcano Casado <3291265+lezcano@users.noreply.github.com>

Remove LowerCholeskyTransform inheritance.

378a0cd

pytorchmergebot force-pushed the positivedefinite branch from 7e8d1df to 378a0cd Compare December 8, 2022 06:17

soumith removed the request for review from neerajprad December 8, 2022 06:19

pytorchmergebot added the Merged label Dec 8, 2022

pytorchmergebot closed this in b485781 Dec 8, 2022

	if type(self).__init__ is Transform.__init__:
	return type(self)(cache_size=cache_size)
	raise NotImplementedError("{}.with_cache is not implemented".format(type(self)))

Add a transform for positive-definite matrices. #76777

Add a transform for positive-definite matrices. #76777

Uh oh!

Conversation

tillahoffmann commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

lezcano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neerajprad commented May 8, 2022

Uh oh!

Uh oh!

nonconvexopt commented May 8, 2022

Uh oh!

tillahoffmann commented Jul 1, 2022

Uh oh!

facebook-github-bot commented Oct 3, 2022

Uh oh!

linux-foundation-easycla bot commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tillahoffmann commented Nov 30, 2022

Uh oh!

lezcano commented Nov 30, 2022

Uh oh!

lezcano left a comment

Choose a reason for hiding this comment

Uh oh!

pytorchmergebot commented Nov 30, 2022

Uh oh!

pytorchmergebot commented Nov 30, 2022

Uh oh!

pytorch-bot bot commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/76777

✅ No Failures

Uh oh!

lezcano commented Nov 30, 2022

Uh oh!

fritzo left a comment

Choose a reason for hiding this comment

Uh oh!

tillahoffmann commented Dec 2, 2022

Uh oh!

fritzo Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lezcano Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

fritzo Dec 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tillahoffmann Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fritzo left a comment

Choose a reason for hiding this comment

Uh oh!

fritzo commented Dec 5, 2022

Uh oh!

lezcano commented Dec 5, 2022

Uh oh!

pytorchmergebot commented Dec 5, 2022

Merge started

Uh oh!

pytorchmergebot commented Dec 5, 2022

Merge failed

Uh oh!

tillahoffmann commented May 3, 2022 •

edited

Loading

facebook-github-bot commented May 3, 2022 •

edited

Loading

linux-foundation-easycla bot commented Oct 3, 2022 •

edited

Loading

pytorch-bot bot commented Nov 30, 2022 •

edited

Loading

fritzo Dec 2, 2022 •

edited

Loading

fritzo Dec 3, 2022 •

edited

Loading

tillahoffmann Dec 5, 2022 •

edited

Loading