Bugfix to MixtureSameFamily's _pad_mixture_dimension #118947

CJMenart · 2024-02-02T01:55:05Z

Fixes Issue #73792

This is a duplicate of pull request. #73864. It's a small bugfix that should have happened a long time ago, but it didn't because I didn't actually follow up with the pull request after originally submitting. That's my bad. Trying to remedy the error.

This contains a fix to _pad_mixture_dimension, which intends to count the number of dimensions in its referent tensors, but accidentally counts the number of elements (and can thus end up creating tensors with potentially thousands of dimensions by mistake). Also contains a single test for the fixed behavior.

cc @albanD

Bugfix to _pad_mixture_dimensions. Now counts number of dimensions instead of number of elements as intended.

Regression test for bugfix to _mad_mixture_dimensions in previous commit.

pytorch-bot · 2024-02-02T01:55:08Z

🔗 Helpful Links

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

📄 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 the bot commands wiki or our office hours

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

✅ No Failures

As of commit 0618d65 with merge base 844a76e ():
💚 Looks good so far! There are no failures yet. 💚

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

linux-foundation-easycla · 2024-02-02T01:55:09Z

The committers listed above are authorized under a signed CLA.

✅ login: CJMenart (f1756d0, 95276b3, 51782e0, 6cff293, e8c2f06, f01b7f4, 0618d65)

test/distributions/test_distributions.py

soulitzer · 2024-02-02T02:17:33Z

test/distributions/test_distributions.py

+    def test_mixture_same_family_shape(self):
+        mix_distribution = Categorical(torch.ones([3,1,3]))
+        component_distribution = torch.distributions.Normal(torch.zeros([3,3,3]), torch.ones([3,3,3]))
+        gmm = MixtureSameFamilyFixed(mix, comp)


whoops looks like you forgot to rename these variables

...My God. This is what I get for not testing the exact same text I commit...chuckleheads like me are why we need devops.

Co-authored-by: Jeffrey Wan <soulitzer@gmail.com>

soulitzer

Thanks!

soulitzer · 2024-02-02T03:33:14Z

@pytorchbot merge

pytorchmergebot · 2024-02-02T03:35:12Z

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team

Raised by workflow job

As caught by CI

soulitzer · 2024-02-02T16:35:33Z

@pytorchbot merge

pytorchmergebot · 2024-02-02T16:37:56Z

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team

Raised by workflow job

CJMenart · 2024-02-03T12:36:03Z

So two checks have failed on the new test. Both are ones with Torch Dynamo. I don't really know anything about Dynamo, or why it's accessing non-existent Tensor attributes. A lot of tests for torch.distributions are marked with this "Don't test this with Dynamo" tag. Trying to figure out if it's appropriate to apply that to the new one.

soulitzer · 2024-02-03T18:29:46Z

So two checks have failed on the new test. Both are ones with Torch Dynamo. I don't really know anything about Dynamo, or why it's accessing non-existent Tensor attributes. A lot of tests for torch.distributions are marked with this "Don't test this with Dynamo" tag. Trying to figure out if it's appropriate to apply that to the new one.

Adding the skip like the other tests do sounds good to me

Not knowledgeable about dynamo really; making a call because test seems simple/reasonable and many other torch.distribution tests skip if Dynamo.

CJMenart · 2024-02-06T12:11:39Z

Tests are passed, do you think this is ready to go?

soulitzer · 2024-02-06T16:21:53Z

Yup, thanks for the follow up

soulitzer · 2024-02-06T16:21:59Z

@pytorchbot merge

pytorchmergebot · 2024-02-06T16:23:59Z

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

Fixes Issue #73792 This is a duplicate of pull request. #73864. It's a small bugfix that should have happened a long time ago, but it didn't because I didn't actually follow up with the pull request after originally submitting. That's my bad. Trying to remedy the error. This contains a fix to _pad_mixture_dimension, which intends to count the number of dimensions in its referent tensors, but accidentally counts the number of elements (and can thus end up creating tensors with potentially thousands of dimensions by mistake). Also contains a single test for the fixed behavior. Co-authored-by: Jeffrey Wan <soulitzer@gmail.com> Pull Request resolved: #118947 Approved by: https://github.com/soulitzer

Fixes Issue pytorch#73792 This is a duplicate of pull request. pytorch#73864. It's a small bugfix that should have happened a long time ago, but it didn't because I didn't actually follow up with the pull request after originally submitting. That's my bad. Trying to remedy the error. This contains a fix to _pad_mixture_dimension, which intends to count the number of dimensions in its referent tensors, but accidentally counts the number of elements (and can thus end up creating tensors with potentially thousands of dimensions by mistake). Also contains a single test for the fixed behavior. Co-authored-by: Jeffrey Wan <soulitzer@gmail.com> Pull Request resolved: pytorch#118947 Approved by: https://github.com/soulitzer

Fixes Issue #73792 This is a duplicate of pull request. #73864. It's a small bugfix that should have happened a long time ago, but it didn't because I didn't actually follow up with the pull request after originally submitting. That's my bad. Trying to remedy the error. This contains a fix to _pad_mixture_dimension, which intends to count the number of dimensions in its referent tensors, but accidentally counts the number of elements (and can thus end up creating tensors with potentially thousands of dimensions by mistake). Also contains a single test for the fixed behavior. Co-authored-by: Jeffrey Wan <soulitzer@gmail.com> Pull Request resolved: #118947 Approved by: https://github.com/soulitzer

CJMenart added 2 commits February 1, 2024 18:50

Bugfix to _pad_mixture_dimensions

f1756d0

Bugfix to _pad_mixture_dimensions. Now counts number of dimensions instead of number of elements as intended.

+test for _pad_dimensions bugfix

95276b3

Regression test for bugfix to _mad_mixture_dimensions in previous commit.

pytorchbot added the open source label Feb 2, 2024

soulitzer reviewed Feb 2, 2024

View reviewed changes

test/distributions/test_distributions.py Outdated Show resolved Hide resolved

soulitzer reviewed Feb 2, 2024

View reviewed changes

soulitzer added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 2, 2024

CJMenart and others added 3 commits February 1, 2024 21:39

Fix var names in test_mixture_same_family_mean_shape

51782e0

MixtureSameFamilyFixed ->MixtureSameFamily

6cff293

remove non-PIP space

e8c2f06

Co-authored-by: Jeffrey Wan <soulitzer@gmail.com>

soulitzer approved these changes Feb 2, 2024

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 2, 2024

pytorchmergebot added the merging label Feb 2, 2024

pytorchmergebot removed the merging label Feb 2, 2024

spaces after commas

f01b7f4

As caught by CI

soulitzer added the module: python frontend For issues relating to PyTorch's Python frontend label Feb 2, 2024

pytorchmergebot added the merging label Feb 2, 2024

pytorchmergebot removed the merging label Feb 2, 2024

soulitzer added the release notes: python_frontend release notes category label Feb 3, 2024

Skip test_mixture_same_family_mean_shape in Dynamo

0618d65

Not knowledgeable about dynamo really; making a call because test seems simple/reasonable and many other torch.distribution tests skip if Dynamo.

pytorchmergebot added the merging label Feb 6, 2024

pytorchmergebot closed this in a77be63 Feb 6, 2024

pytorchmergebot added Merged and removed merging labels Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix to MixtureSameFamily's _pad_mixture_dimension #118947

Bugfix to MixtureSameFamily's _pad_mixture_dimension #118947

CJMenart commented Feb 2, 2024 •

edited by pytorch-bot bot

pytorch-bot bot commented Feb 2, 2024 •

edited

linux-foundation-easycla bot commented Feb 2, 2024 •

edited

soulitzer Feb 2, 2024

CJMenart Feb 2, 2024

soulitzer left a comment

soulitzer commented Feb 2, 2024

pytorchmergebot commented Feb 2, 2024

soulitzer commented Feb 2, 2024

pytorchmergebot commented Feb 2, 2024

CJMenart commented Feb 3, 2024

soulitzer commented Feb 3, 2024

CJMenart commented Feb 6, 2024

soulitzer commented Feb 6, 2024

soulitzer commented Feb 6, 2024

pytorchmergebot commented Feb 6, 2024

Bugfix to MixtureSameFamily's _pad_mixture_dimension #118947

Bugfix to MixtureSameFamily's _pad_mixture_dimension #118947

Conversation

CJMenart commented Feb 2, 2024 • edited by pytorch-bot bot

pytorch-bot bot commented Feb 2, 2024 • edited

🔗 Helpful Links

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

✅ No Failures

linux-foundation-easycla bot commented Feb 2, 2024 • edited

soulitzer Feb 2, 2024

Choose a reason for hiding this comment

CJMenart Feb 2, 2024

Choose a reason for hiding this comment

soulitzer left a comment

Choose a reason for hiding this comment

soulitzer commented Feb 2, 2024

pytorchmergebot commented Feb 2, 2024

Merge failed

soulitzer commented Feb 2, 2024

pytorchmergebot commented Feb 2, 2024

Merge failed

CJMenart commented Feb 3, 2024

soulitzer commented Feb 3, 2024

CJMenart commented Feb 6, 2024

soulitzer commented Feb 6, 2024

soulitzer commented Feb 6, 2024

pytorchmergebot commented Feb 6, 2024

Merge started

CJMenart commented Feb 2, 2024 •

edited by pytorch-bot bot

pytorch-bot bot commented Feb 2, 2024 •

edited

linux-foundation-easycla bot commented Feb 2, 2024 •

edited