Skip to content

Refactor native/cpu/zmath.h#38037

Closed
zasdfgbnm wants to merge 2 commits intomasterfrom
zmath.h
Closed

Refactor native/cpu/zmath.h#38037
zasdfgbnm wants to merge 2 commits intomasterfrom
zmath.h

Conversation

@zasdfgbnm
Copy link
Copy Markdown
Collaborator

There is now a zmath.h and zmath_std.h, where the latter is the copy-paste of the original zmath.h and supporting std::complex, and zmath.h is for supporting c10::complex. zmath_std.h will be removed eventually.

@zasdfgbnm zasdfgbnm added the module: complex Related to complex number support in PyTorch label May 7, 2020
@zasdfgbnm zasdfgbnm mentioned this pull request May 7, 2020
@anjali411
Copy link
Copy Markdown
Contributor

why do we need zmath with c10::complex? Correct me if I am wrong, but isn't zmath just used because we need thrust::complex on cuda instead of std::complex which was the defined ctype for complex earlier

@zasdfgbnm
Copy link
Copy Markdown
Collaborator Author

@anjali411 There is a native/cuda/zmath.h and native/cpu/zmath.h, the cuda zmath is just for wrapping std::complex into thrust::complex, but on cpu zmath, it has more: for example, it contains some helper such as zabs which is used to wrap the scalar_t in the implementation torch.min.

Although I doubt that torch.min is taking the min of complex according to its norm does not make much sense, it is still good to have this refactor to unblock the migration of dispatch macros, and take care of the semantics of operators as a separate task.

@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented May 7, 2020

💊 CI failures summary and remediations

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


None of the CI failures appear to be your fault 💚



🚧 2 fixed upstream failures:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)

If your commit is older than viable/strict:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.


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 3 times.

template<>
constexpr std::complex<double> real_impl <std::complex<double>> (std::complex<double> z) {
return std::complex<double>(z.real(), 0.0);
constexpr c10::complex<double> real_impl <c10::complex<double>> (c10::complex<double> z) {
Copy link
Copy Markdown
Collaborator Author

@zasdfgbnm zasdfgbnm May 8, 2020

Choose a reason for hiding this comment

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

Looks like this is not needed, std::real should work. @anjali411 Please hold on and I will let you know when this is ready.

Edit: No, I think this PR is fine. std::real is not defined for BFloat and Half, so we still need this real_impl trick.

template<>
inline std::complex<float> zabs <std::complex<float>> (std::complex<float> z) {
return std::complex<float>(std::abs(z));
inline c10::complex<float> zabs <c10::complex<float>> (c10::complex<float> z) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This should be still needed.

Copy link
Copy Markdown
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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@anjali411 merged this pull request in bf499cc.

@zasdfgbnm zasdfgbnm deleted the zmath.h branch May 12, 2020 06:29
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
Summary:
There is now a `zmath.h` and `zmath_std.h`, where the latter is the copy-paste of the original `zmath.h` and supporting `std::complex`, and `zmath.h` is for supporting `c10::complex`. `zmath_std.h` will be removed eventually.
Pull Request resolved: pytorch#38037

Differential Revision: D21518177

Pulled By: anjali411

fbshipit-source-id: 18552e955dc31f95870f34962d709de0444804f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: complex Related to complex number support in PyTorch open source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants