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.

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

Fix gumbel cdf #91698

Closed
wants to merge 4 commits into from
Closed

Fix gumbel cdf #91698

wants to merge 4 commits into from

Conversation

vfonov
Copy link
Contributor

@vfonov vfonov commented Jan 4, 2023

Fix Gumbel.cdf function.

Description
When transformed parameters is outside of the support of underlying Uniform distribution. This makes behavior of Gumbel.cdf consistent with other TransformedDistribution that pass value of validate_args to the base distribution.

Issue
running Gumbel(0.0,1.0,validate_args=False).cdf(20.0) would cause ValueError exception from _validate_sample

Testing
Test was added to the test_distributions.py to check if Gumbel(0.0,1.0,validate_args=False).cdf(20.0) successfully returns 1.0

This is a second attempt to push changes , after #82488

cc @fritzo @neerajprad @alicanb @nikitaved

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 4, 2023

🔗 Helpful Links

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

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

✅ No Failures

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

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

@zou3519 zou3519 added the module: distributions Related to torch.distributions label Jan 4, 2023
@zou3519 zou3519 requested a review from fritzo January 4, 2023 15:22
@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 4, 2023
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, thanks for fixing this!

@fritzo
Copy link
Collaborator

fritzo commented Jan 5, 2023

@zou3519 could you merge this please? I'm unsure of the current merge process

@github-actions
Copy link

github-actions bot commented Mar 6, 2023

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Mar 6, 2023
@zou3519
Copy link
Contributor

zou3519 commented Mar 7, 2023

Sorry, I was out of town for a while and just saw this now. Let me try to rebase (automatically first, then manually) and then I'll get this merged.

@zou3519 zou3519 removed the Stale label Mar 7, 2023
@zou3519
Copy link
Contributor

zou3519 commented Mar 7, 2023

@pytorchbot help

@pytorch-bot
Copy link

pytorch-bot bot commented Mar 7, 2023

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: argument command: invalid choice: 'help' (choose from 'merge', 'revert', 'rebase', 'label', 'drci')

usage: @pytorchbot [-h] {merge,revert,rebase,label,drci} ...

Try @pytorchbot --help for more info.

@zou3519
Copy link
Contributor

zou3519 commented Mar 7, 2023

@pytorchbot rebase

@zou3519 zou3519 self-assigned this Mar 7, 2023
@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

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

@zou3519 zou3519 added the ciflow/trunk Trigger trunk jobs on your pull request label Mar 7, 2023
@zou3519
Copy link
Contributor

zou3519 commented Mar 7, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a 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.

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

@zou3519 zou3519 added the release notes: python_frontend release notes category label Mar 7, 2023
@zou3519
Copy link
Contributor

zou3519 commented Mar 7, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

ydwu4 pushed a commit to ydwu4/pytorch that referenced this pull request Mar 10, 2023
Fix `Gumbel.cdf` function.

**Description**
When transformed parameters is outside of the support of underlying Uniform distribution. This makes behavior of `Gumbel.cdf` consistent with other `TransformedDistribution` that pass value of validate_args to the base distribution.

**Issue**
running `Gumbel(0.0,1.0,validate_args=False).cdf(20.0)` would cause `ValueError` exception from `_validate_sample`

**Testing**
Test was added to the `test_distributions.py` to check if `Gumbel(0.0,1.0,validate_args=False).cdf(20.0)` successfully returns `1.0`

This is a second attempt to push changes , after pytorch#82488

Pull Request resolved: pytorch#91698
Approved by: https://github.com/fritzo, https://github.com/zou3519
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 12, 2023
Fix `Gumbel.cdf` function.

**Description**
When transformed parameters is outside of the support of underlying Uniform distribution. This makes behavior of `Gumbel.cdf` consistent with other `TransformedDistribution` that pass value of validate_args to the base distribution.

**Issue**
running `Gumbel(0.0,1.0,validate_args=False).cdf(20.0)` would cause `ValueError` exception from `_validate_sample`

**Testing**
Test was added to the `test_distributions.py` to check if `Gumbel(0.0,1.0,validate_args=False).cdf(20.0)` successfully returns `1.0`

This is a second attempt to push changes , after pytorch/pytorch#82488

Pull Request resolved: pytorch/pytorch#91698
Approved by: https://github.com/fritzo, https://github.com/zou3519
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 12, 2023
Fix `Gumbel.cdf` function.

**Description**
When transformed parameters is outside of the support of underlying Uniform distribution. This makes behavior of `Gumbel.cdf` consistent with other `TransformedDistribution` that pass value of validate_args to the base distribution.

**Issue**
running `Gumbel(0.0,1.0,validate_args=False).cdf(20.0)` would cause `ValueError` exception from `_validate_sample`

**Testing**
Test was added to the `test_distributions.py` to check if `Gumbel(0.0,1.0,validate_args=False).cdf(20.0)` successfully returns `1.0`

This is a second attempt to push changes , after pytorch/pytorch#82488

Pull Request resolved: pytorch/pytorch#91698
Approved by: https://github.com/fritzo, https://github.com/zou3519
ydwu4 added a commit to ydwu4/pytorch that referenced this pull request Mar 13, 2023
Fix `Gumbel.cdf` function.

**Description**
When transformed parameters is outside of the support of underlying Uniform distribution. This makes behavior of `Gumbel.cdf` consistent with other `TransformedDistribution` that pass value of validate_args to the base distribution.

**Issue**
running `Gumbel(0.0,1.0,validate_args=False).cdf(20.0)` would cause `ValueError` exception from `_validate_sample`

**Testing**
Test was added to the `test_distributions.py` to check if `Gumbel(0.0,1.0,validate_args=False).cdf(20.0)` successfully returns `1.0`

This is a second attempt to push changes , after pytorch#82488

Pull Request resolved: pytorch#91698
Approved by: https://github.com/fritzo, https://github.com/zou3519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: distributions Related to torch.distributions open source release notes: python_frontend release notes category 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.

None yet

5 participants