Skip to content

Conversation

zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented May 16, 2020

Main:

  • c10::complex is refactored: it no longer uses inheritance to specialize constructors, but using SFINAE instead. This implementation is cleaner and avoids some compiler bugs.
  • c10::Scalar is cleaned up: it no longer needs to store complex as double z[2], c10::complex<double> will work.

Other cleanups:

  • numeric_limits of c10::complex is moved to complex_utils.h
  • the variable in c10::complex storing real and imag is changed from storage[2] to real_ and imag_
  • remove the c10:: before complex when in c10 namespace

@zasdfgbnm zasdfgbnm changed the title Refactor c10::complex and cleanup c10::Scalar [WIP] Refactor c10::complex and cleanup c10::Scalar May 16, 2020
@dr-ci
Copy link

dr-ci bot commented May 16, 2020

💊 CI failures summary and remediations

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


None of the CI failures appear to be your fault 💚



🚧 2 ongoing upstream failures:

These were probably caused by upstream breakages that are not fixed 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 on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 1 time.

@zasdfgbnm zasdfgbnm changed the title [WIP] Refactor c10::complex and cleanup c10::Scalar Refactor c10::complex and cleanup c10::Scalar May 16, 2020
@zasdfgbnm zasdfgbnm requested review from anjali411 and ezyang May 16, 2020 10:36
using value_type = T;

T storage[2] = {T(), T()};
T real_ = T(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we name them real and imag to keep it similar to the (TBA) python API

Copy link
Collaborator Author

@zasdfgbnm zasdfgbnm May 16, 2020

Choose a reason for hiding this comment

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

No, there is already a method named real

@ailzhang ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 21, 2020
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@anjali411 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@anjali411 merged this pull request in 928ce29.

@anjali411
Copy link
Contributor

@zasdfgbnm this PR was reverted because it broke master. The OSS ci was based on a branch that was 15 days old.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants