-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Optimize implementation of torch.pow #46830
Conversation
b58e34a
to
de7da46
Compare
💊 CI failures summary and remediationsAs of commit dfe8670175 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
e0f9115
to
0b9deb5
Compare
adding @zou3519, as this PR is adding named tensor support to the operator |
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.
okey dokey
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.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@anjali411 I just realized that this PR intersects with your fix for the accidental complex promotion. Do you think you could work these changes into your patchset in some way? Otherwise I'll wait for your fix to land and rebase this on top of yours. |
0b9deb5
to
37c25a3
Compare
I rebased this PR since it has been a while. BTW, if there is anything else I can do for this PR, I‘d like to be the help. |
@ezyang sorry I completely missed this PR. let me merge my |
37c25a3
to
929ad85
Compare
929ad85
to
d159ac7
Compare
Hi @Kiyosora can you rebase this PR on the latest master? |
d159ac7
to
4da9ade
Compare
Sure, I've rebase this PR on the latest master. @anjali411 |
aten/src/ATen/native/Pow.cpp
Outdated
result.resize_as_(base).copy_(base); | ||
auto exponent = (exp.isComplex()) ? exp.toComplexDouble() : exp.toDouble(); | ||
|
||
if (exponent == 0.0) { |
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.
@Kiyosora can you please retain this check to exp.equal(0.0)
(as in master)?
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.
Addressed!
aten/src/ATen/native/Pow.cpp
Outdated
result.resize_as_(base).fill_(1); | ||
} else if (exp.equal(1.0)) { | ||
result.resize_as_(base).copy_(base); | ||
auto exponent = (exp.isComplex()) ? exp.toComplexDouble() : exp.toDouble(); |
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.
this is no longer needed. let's remove this!
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.
Addressed!
aten/src/ATen/native/Pow.cpp
Outdated
resize_output(result, base.sizes()); | ||
result.fill_(1); | ||
namedinference::propagate_names(result, base); | ||
} else if (exponent == 1.0) { |
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.
same comment as above
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.
Addressed!
aten/src/ATen/native/Pow.cpp
Outdated
} else if (!base.isComplex() && base.toDouble() == 1.0) { | ||
result.resize_as_(exp).fill_(1); | ||
|
||
auto exponent = (base.isComplex()) ? base.toComplexDouble() : base.toDouble(); |
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.
same comment as above
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.
Addressed!
aten/src/ATen/native/Pow.cpp
Outdated
|
||
auto exponent = (base.isComplex()) ? base.toComplexDouble() : base.toDouble(); | ||
|
||
if (exponent == 1.0) { |
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.
same as above
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.
Addressed!
dfe8670
to
779026a
Compare
779026a
to
d014a9f
Compare
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 thanks @Kiyosora
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.
@anjali411 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@anjali411 merged this pull request in d140ca8. |
resize_output
instead ofresize_as
native_functions.yaml
, move the inplace variantpow_
next to the otherpow
entries