-
Notifications
You must be signed in to change notification settings - Fork 25.2k
OpInfo: true_divide and minor fix #59154
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
OpInfo: true_divide and minor fix #59154
Conversation
💊 CI failures summary and remediationsAs of commit c08c481 (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
❄️ 1 failure tentatively classified as flakybut reruns have not yet been triggered to confirm:
|
('true_divide', (S, S, S), (uniform_scalar(0.1),), 'scalar_broadcast_rhs', (True,)), | ||
('true_divide', (), (uniform_scalar(0.1),), 'scalar_broadcast_lhs', (True,)), | ||
('true_divide', torch.rand(S, S, S) + 1e-1, (3.14,), 'constant', (True,)), | ||
('true_divide', uniform_scalar(1e-1, requires_grad=True), (3.14,), 'scalar_constant', (True,)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cases covered by binary_pwise,
pytorch/torch/testing/_internal/common_methods_invocations.py
Lines 727 to 741 in 41054f2
scalar = 3.14 + 3.14j if dtype.is_complex else (3.14 if dtype.is_floating_point else 3) | |
scalar = 1 if dtype is torch.bool else scalar | |
tests_list = [ | |
((S, S, S), (S, S, S), False), | |
((S, S, S), (S, S), False), | |
((), (), False), | |
((S, S, S), (), False), | |
((S, S, S), scalar, False), | |
((), scalar, False) | |
] | |
tests_with_lhs_broadcasting = [ | |
((S, S), (S, S, S), True), | |
((), (S, S, S), True), | |
((S, 1, S), (M, S), True), | |
] |
Another NNC issue:
cc @Chillee Also we should rename that test to be more specific, like "test_nnc_unsupported" |
And can we move it into test_ops.py? That way people only have to run one test file when adding an OpInfo. The jit support tests are also in test_ops.py, for example. |
real = torch.tensor(torch.finfo(float_dtype).eps, device=device, dtype=dtype) | ||
imag = torch.tensor(torch.finfo(float_dtype).eps, device=device, dtype=dtype) | ||
replace_with = torch.complex(real, imag) | ||
float_eps = torch.tensor(torch.finfo(float_dtype).eps, device=device, dtype=float_dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @kshitij12345. This needs a fix for the NNC test and a small comment update.
I also left a couple notes for @Chillee to move that test into test_ops.py (if possible), improve its naming, and (these notes are elsewhere) add metadata to OpInfos for whether they support NNC or not. That should make it easier to work with that test. But we should do any of those things in this PR.
Is div next?
These changes sound fine to me - the main consideration is just maximizing the ease of adding new OpInfos + maintainibility of these OpInfos. I'd be glad to put up a PR for it, although I'm PTO the next 2 weeks, so it's possible I won't get to it (unless I do it now... maybe). Feel free to move the code to where you'd like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @kshitij12345!
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Enjoy your PTO! I filed #59185 (comment) to track this |
Summary: Reference: pytorch#54261 Pull Request resolved: pytorch#59154 Reviewed By: ngimel Differential Revision: D28780115 Pulled By: mruberry fbshipit-source-id: 91e254698597fa0c7d4df6053ec017a85e180304
Summary: Reference: pytorch#54261 Depends on: pytorch#59154 Pull Request resolved: pytorch#59173 Reviewed By: ngimel Differential Revision: D28785178 Pulled By: mruberry fbshipit-source-id: 902310f2d77e499a2355a23b2d5a8c0b21b8c5bb
Reference: #54261