Returns float from complex angle #36896
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mruberry
added
module: complex
anjali411
reviewed
Apr 21, 2020
anjali411
reviewed
Apr 21, 2020
Comment on lines
+91
to
+92
| Tensor result = at::empty({0}, self.options()); | ||
| return out_impl(result, self); |
There was a problem hiding this comment.
why do we need this? don't we only call this function for complex tensors?
There was a problem hiding this comment.
ahhh I see. you are calling this method from abs for non complex tensors as well. I think it's a little misleading to call this method for non complex inputs because of its name. So, I think we can do two things here:
- change the name of the function to make it more generic.
- call this method for only complex inputs.
Whatever you prefer.
There was a problem hiding this comment.
maybe just add a word or two like
unary_op_impl_with_complex_to_float
There was a problem hiding this comment.
I still think it will not be intuitive to a developer who is not familiar with this PR and looks at this code and it could be confusing. I would caution on the side of either removing "complex_to_float" from the name or adding descriptive comments specifically mentioning this function is also being used for real values.
There was a problem hiding this comment.
I updated the comment.
anjali411
reviewed
Apr 21, 2020
| @@ -53,6 +80,18 @@ static inline Tensor unary_op_impl(const Tensor& self, OutImpl& out_impl) { | |||
| return out_impl(result, self); | |||
| } | |||
|
|
|||
| template <typename OutImpl> | |||
| static inline Tensor unary_op_impl_complex_to_float(const Tensor& self, OutImpl& out_impl) { | |||
| if (self.is_complex()) { | |||
There was a problem hiding this comment.
maybe add TORCH_INTERNAL_ASSERT(self.is_complex());
anjali411
requested changes
Apr 21, 2020
gchanan
reviewed
Apr 21, 2020
gchanan
approved these changes
Apr 21, 2020
💊 Build failures summary and remediationsAs of commit 5ee8db2 (more details on the Dr. CI page):
🕵️ 2 new failures recognized by patternsThe following build failures do not appear to be due to upstream breakages:
|
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates angle to return a float tensor, by default, when given complex inputs. This behavior is compatible with Python, NumPy, and C++. The implementation follows the former implementation for complex abs, extracting the logic into a common function for both abs and angle.
The test for complex abs's behavior in test_type_promotion.py is updated to also test the behavior of complex angle by comparing its results to NumPy's.