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.

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

Add complex IValues #50883

Closed
wants to merge 6 commits into from
Closed

Add complex IValues #50883

wants to merge 6 commits into from

Conversation

anjali411
Copy link
Contributor

@anjali411 anjali411 commented Jan 21, 2021 •
edited

Stack from ghstack:

Differential Revision: D26003682

@anjali411
Add complex IValues
32c8d28 
[ghstack-poisoned]
This was referenced Jan 21, 2021
Closed
Closed
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 21, 2021 •
edited

💊 CI failures summary and remediations

As of commit c42bd1a (more details on the Dr. CI page):

  • 4/4 failures possibly* introduced in this PR
    • 2/4 non-CircleCI failure(s)

2 failures not recognized by patterns:

Job Step Action
CircleCI pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test2 Run tests 🔁 rerun
CircleCI pytorch_windows_vs2019_py36_cuda10.1_test2 Unknown 🔁 rerun

Extra GitHub checks: 1 failed

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.

@anjali411 anjali411 mentioned this pull request Jan 21, 2021
Closed
SplitInfinity
SplitInfinity reviewed Jan 21, 2021
View reviewed changes
Copy link

@SplitInfinity SplitInfinity left a comment

Choose a reason for hiding this comment

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

Nice! After further discussion with the JIT team about the place of ComplexDoubleType in the type system, we concluded that it makes more sense for it to be a subtype of only Any, and not of NumberType. Other than that, the rest looks good!

aten/src/ATen/core/jit_type.h Show resolved Hide resolved
aten/src/ATen/core/jit_type.h Show resolved Hide resolved
aten/src/ATen/core/jit_type.h Outdated Show resolved Hide resolved
aten/src/ATen/core/jit_type.h Outdated Show resolved Hide resolved
anjali411 added a commit that referenced this pull request Jan 21, 2021
@anjali411
Add complex IValues
490cb4f 
ghstack-source-id: a9c8c5ab3dda0c139518f72e4e1adc2c3048acb6
Pull Request resolved: #50883
SplitInfinity
SplitInfinity reviewed Jan 21, 2021
View reviewed changes
aten/src/ATen/test/type_test.cpp
@@ -175,8 +175,8 @@ class OneForward(Interface):

TEST(TypeEquality, TupleEquality) {
// Tuples should be structurally typed
auto type = TupleType::create({IntType::get(), TensorType::get(), FloatType::get()});
auto type2 = TupleType::create({IntType::get(), TensorType::get(), FloatType::get()});
auto type = TupleType::create({IntType::get(), TensorType::get(), FloatType::get(), ComplexDoubleType::get()});
Copy link

Choose a reason for hiding this comment

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

Are these changes for testing ComplexDoubleType::operator==?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah and also indirectly testing ComplexDoubleType::get()

SplitInfinity
SplitInfinity approved these changes Jan 21, 2021
View reviewed changes
Copy link

@SplitInfinity SplitInfinity left a comment •
edited

Choose a reason for hiding this comment

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

Let's make sure to follow up on the correct place for ComplexDoubleType in the JIT type system and fix it if needed in the future.

Approval contingent on tests passing.

@anjali411
Copy link
Contributor Author

Nice! After further discussion with the JIT team about the place of ComplexDoubleType in the type system, we concluded that it makes more sense for it to be a subtype of only Any, and not of NumberType. Other than that, the rest looks good!

Synced offline: It probably makes sense for ComplexDoubleType to be a subtype of NumberType since we can have complex scalars and functions like torch.full, torch.index_fill etc. can take complex numbers as input.

@facebook-github-bot
Copy link
Contributor

@anjali411 merged this pull request in 9ac30d9.

@facebook-github-bot facebook-github-bot deleted the gh/anjali411/87/head branch January 26, 2021 15:21
@anjali411 anjali411 added the module: complex Related to complex number support in PyTorch label Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SplitInfinity SplitInfinity SplitInfinity approved these changes

Assignees
No one assigned
Labels
cla signed Merged module: complex Related to complex number support in PyTorch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@anjali411 @facebook-github-bot @SplitInfinity