Skip to content

[Inductor] added aten.cauchy_ decomp #92047

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
Closed
wants to merge 27 commits into from
Closed

[Inductor] added aten.cauchy_ decomp #92047

wants to merge 27 commits into from

Conversation

@min-jean-cho
Copy link
Collaborator

@min-jean-cho min-jean-cho commented Jan 11, 2023 •
edited by pytorch-bot bot
Loading

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 11, 2023 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

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

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

@min-jean-cho min-jean-cho requested a review from jgong5 January 11, 2023 23:14
jgong5
jgong5 reviewed Jan 13, 2023
View reviewed changes
torch/_decomp/decompositions.py Outdated
@register_decomposition(aten.cauchy_)
def cauchy_(self, median=0, gamma=1, generator=None):
assert generator is None
M_PI = 3.141592653589793238462643383279502
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not using math.pi?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, we should use math.pi

@min-jean-cho min-jean-cho requested a review from jgong5 January 17, 2023 22:00
@min-jean-cho min-jean-cho marked this pull request as ready for review January 17, 2023 22:45
jgong5
jgong5 approved these changes Jan 18, 2023
View reviewed changes
Copy link
Collaborator

@jgong5 jgong5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has test covered?

@lezcano
Copy link
Collaborator

lezcano commented Jan 18, 2023

Could you add the relevant OpInfo test?

@ngimel
Copy link
Collaborator

ngimel commented Jan 22, 2023

Let's split this into first adding tan lowering to inductor, and then add cauchy decomposition that'll also require some changes to vmap testing. For inductot tan lowering you'd need to use libdevice for triton, see other math functions for examples of how this is done.

@min-jean-cho min-jean-cho mentioned this pull request Jan 23, 2023
Closed
jansel
jansel requested changes Jan 27, 2023
View reviewed changes
test/inductor/test_torchinductor_opinfo.py Outdated
"bernoulli": {f32, f64},
"bincount": {i32, i64},
"bucketize": {b8, f16, f32, f64, i32, i64},
"cauchy": {f16, f64},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this add expected failures?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jansel, they shouldn't. The in-place sampling ops cause graph breaks with nopython=True. I'll remove the graph breaks prior to these decomps. Please see discussion here #92709. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's the story with inductor opinfo testing now? It can't be passing because results for random sampling would be different between eager and inductor. Is the test not run?

Copy link
Collaborator Author

@min-jean-cho min-jean-cho Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be passing because results for random sampling would be different between eager and inductor. Is the test not run?

Yes, the tests will be skipped because eager --vs-- inductor randomness are not comparable. (The tests are not skipped due to graph break of inplace ops; graph break has been resolved in #93243.)
But I don't know why some inductor randomness tests (eg: uniform cpu float32, float64) have passed. https://github.com/pytorch/pytorch/pull/93243/files#r1094149843

I'm not sure how generator is partially supported in inductor.

@ngimel ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 28, 2023
@min-jean-cho min-jean-cho mentioned this pull request Jan 30, 2023
Closed
pytorchmergebot pushed a commit that referenced this pull request Feb 3, 2023
@min-jean-cho @pytorchmergebot
cauchy_ few fixes (1) check gamma > 0 (2) better dtype error log (#93314
6e1cfcd 
)

Related #92047

(1) `torch.Tensor.cauchy_` is missing check for `gamma > 0` (`torch.distributions.cauchy.Cauchy` correctly checks  `gamma > 0`).
(2) add better error log on dtype similar to exponential_

Pull Request resolved: #93314
Approved by: https://github.com/jgong5, https://github.com/fritzo, https://github.com/lezcano
@min-jean-cho
Copy link
Collaborator Author

min-jean-cho commented Feb 8, 2023 •
edited
Loading

[Cauchy] Hi all, thank you for the review. I see all tests have passed now - I've made few minor changes since the last approvals, could you help have a final scan ?

@min-jean-cho
Copy link
Collaborator Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 8, 2023
@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

This was referenced Feb 9, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jansel jansel jansel approved these changes

@lezcano lezcano lezcano approved these changes

@jgong5 jgong5 jgong5 approved these changes

@ngimel ngimel ngimel approved these changes

@mruberry mruberry Awaiting requested review from mruberry

@mrshenli mrshenli Awaiting requested review from mrshenli

@zhaojuanmao zhaojuanmao Awaiting requested review from zhaojuanmao

@rohan-varma rohan-varma Awaiting requested review from rohan-varma

@H-Huang H-Huang Awaiting requested review from H-Huang

@awgu awgu Awaiting requested review from awgu

@kwen2501 kwen2501 Awaiting requested review from kwen2501

@wanchaol wanchaol Awaiting requested review from wanchaol

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor 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.

[RFC] [Inductor] decompose aten.cauchy_

7 participants

@min-jean-cho @lezcano @ngimel @pytorchmergebot @jansel @jgong5 @pytorchbot