Skip to content

Conversation

imaginary-person
Copy link
Contributor

@imaginary-person imaginary-person commented Jan 24, 2021

Added the functionality desired in #50789.

Summary

  1. Added support for pow() on CPU for float16 (Half) and bfloat16 types.
    Both pow(Tensor, Scalar) and pow(Tensor, Tensor) are now supported for the aforementioned types.
    However autograd isn't supported for Float16 on CPU yet, as log_vml_cpu can't be enabled for it.
  2. @heitorschueroff added pow_tensor_scalar_optimized_kernel to refactor & simplify PowKernel.cpp.
    It provides a common path for all the complex types & floating point types (except Float16, due to lack of complete AVX2 vectorization support for it). It replaced code that had previously been duplicated for (float, double) and complex types,
    so PowKernel.cpp looks a lot cleaner now.
  3. Enabled (unskipped) some tests for erf, erfc,erfinv, linalg.norm and linalg.vector.norm which were being skipped earlier due to pow() not having been implemented for float16 & bfloat16.
  4. Added an OpInfo for pow() & enabled some test cases for pow().
  5. Extended the coverage of existing tests for pow in test_binary_ufuncs.py in order to enable comparison with numpy, even with discontiguous tensors, and added a test to ensure that a runtime error is raised for pow's inplace variant if resizing the base tensor is required during its invocation.
  6. Added float16 & bfloat16 to square's dtype lists in its UnaryUfuncInfo.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 24, 2021

💊 CI failures summary and remediations

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


  • 4/4 failures introduced in this PR

4 failures not recognized by patterns:

Job Step Action
GitHub Actions clang-tidy Unknown 🔁 rerun
GitHub Actions flake8-py3 Unknown 🔁 rerun
GitHub Actions mypy Unknown 🔁 rerun
GitHub Actions quick-checks Unknown 🔁 rerun

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.

@imaginary-person

This comment has been minimized.

@imaginary-person

This comment has been minimized.

@imaginary-person

This comment has been minimized.

@imaginary-person imaginary-person marked this pull request as ready for review January 25, 2021 17:29
@mruberry mruberry self-requested a review January 25, 2021 17:36
@mruberry mruberry added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 25, 2021
@mruberry
Copy link
Collaborator

@imaginary-person do me a favor and ping me tomorrow or Wednesday on this PR and I'll take a look.

@imaginary-person

This comment has been minimized.

@imaginary-person imaginary-person marked this pull request as draft January 25, 2021 17:43
@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #50999 (227e771) into master (9e6877c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 227e771 differs from pull request most recent head 7274779. Consider uploading reports for the commit 7274779 to get more accurate results

@@           Coverage Diff           @@
##           master   #50999   +/-   ##
=======================================
  Coverage   77.45%   77.46%           
=======================================
  Files        1894     1894           
  Lines      186403   186437   +34     
=======================================
+ Hits       144374   144416   +42     
+ Misses      42029    42021    -8     

@imaginary-person

This comment has been minimized.

@imaginary-person imaginary-person marked this pull request as ready for review January 26, 2021 04:52
@imaginary-person

This comment has been minimized.

@imaginary-person imaginary-person marked this pull request as draft January 28, 2021 18:49
@imaginary-person imaginary-person force-pushed the imaginary-person-pow-float16-bfloat16 branch from 96e2d44 to 5c57e01 Compare January 30, 2021 00:52
@imaginary-person imaginary-person marked this pull request as ready for review January 30, 2021 03:59
@imaginary-person

This comment has been minimized.

Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Hey @imaginary-person!

There's a lot of good stuff going on here. An OpInfo needs to be added for pow, and the legacy pow sample inputs removed from method_tests in common_methods_invocations.py. Implementing an OpInfo for pow will also let you remove those legacy test_torch.py pow sample inputs and not have to worry about updating them.

As for the pow function itself, can we really not simplify the implementation to avoid tripling its size? pow has been an extremely tricky function to get right, and I'm worried about making it even harder to maintain.

Copy link
Contributor

@heitorschueroff heitorschueroff left a comment

Choose a reason for hiding this comment

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

LGTM! @anjali411 do you want to check the complex part before I land this?

Thanks for the great work on this PR @imaginary-person

@imaginary-person
Copy link
Contributor Author

@heitorschueroff, thanks a lot for your & @mruberry's enormous help & patience with this PR!
BTW, I haven't unskipped test_int_and_float_pow for other dtypes (except int8) on ROCm yet.

@facebook-github-bot
Copy link
Contributor

@heitorschueroff has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Removed scalar_t = decltype(c10::impl::ScalarTypeToCPPType<ScalarType::Half>::t), as the AT_DISPATCH_FLOATING_TYPES_AND macro does this assignment anyway.
@facebook-github-bot
Copy link
Contributor

@heitorschueroff has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@heitorschueroff heitorschueroff requested a review from ngimel April 1, 2021 17:47
Copy link
Collaborator

@ngimel ngimel left a comment

Choose a reason for hiding this comment

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

lgtm

@facebook-github-bot
Copy link
Contributor

@heitorschueroff merged this pull request in 6d030c1.

@heitorschueroff
Copy link
Contributor

@imaginary-person This was not a trivial task and the refactor + tests you added are a great contribution to the project. Thank you!

@peterjc123
Copy link
Collaborator

peterjc123 commented Apr 3, 2021

This PR is causing the majority of the jobs to fail in CI, as can be observed in https://ezyang.github.io/pytorch-ci-hud/build2/pytorch-master.
Example test runs:
https://app.circleci.com/pipelines/github/pytorch/pytorch/295605/workflows/6f0cdf22-772d-4bf4-9ee2-9ad41ed2a668/jobs/12095451
https://app.circleci.com/pipelines/github/pytorch/pytorch/295605/workflows/6f0cdf22-772d-4bf4-9ee2-9ad41ed2a668/jobs/12096482
https://app.circleci.com/pipelines/github/pytorch/pytorch/295605/workflows/6f0cdf22-772d-4bf4-9ee2-9ad41ed2a668/jobs/12096722

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by 8377e62.

@imaginary-person
Copy link
Contributor Author

imaginary-person commented Apr 3, 2021

@malfet @peterjc123,

Sorry for the inconvenience!
#54949 has affected the failures. I haven't checked why yet.
I'll check & re-submit this PR for relanding after making any required changes.

EDIT: #54949 caught a bug in this PR. For 4 of the 9 bool sample inputs, the dtype wasn't actually bool! 😞
So, this line had to be modified, with dtype being changed to dtype=torch.bool:

https://github.com/imaginary-person/pytorch-1/blob/e451285877225d3079faf5a8001e52ca22dc64ab/torch/testing/_internal/common_methods_invocations.py#L1607

Get changes from main repo
@imaginary-person

This comment has been minimized.

facebook-github-bot pushed a commit that referenced this pull request Apr 13, 2021
Summary:
#### Reason for relanding
Line 1607 of `torch/testing/_internal/common_methods_invocations.py` of #50999  had `dtype` instead of `dtype=torch.bool`, so 4 of the 9 sample inputs for `bool` had incorrect dtype. This bug was caught by #54949.

1. Added support for pow() on CPU for `float16` (`Half`) and `bfloat16` types.
Both `pow(Tensor, Scalar)` and `pow(Tensor, Tensor)` are now supported for the aforementioned types.
However autograd isn't supported for `Float16` on CPU yet, as `log_vml_cpu` can't be enabled for it.
2. heitorschueroff added `pow_tensor_scalar_optimized_kernel` to refactor & simplify `PowKernel.cpp`.
It provides a common path for all the complex types & floating point types (except Float16, due to lack of complete AVX2 vectorization support for it).  It replaced code that had previously been duplicated for (float, double) and complex types,
so PowKernel.cpp looks a lot cleaner now.
3. Enabled (unskipped) some tests for `erf`, `erfc`,`erfinv`, `tan` and `linalg.vector.norm` which were being skipped earlier due to `pow()` not having been implemented for `float16` & `bfloat16`.
4. Added an OpInfo for `pow()` & enabled some test cases for `pow()`.
5. Extended the coverage of existing tests for `pow` in `test_binary_ufuncs.py` in order to enable comparison with `numpy`, even with discontiguous tensors, and added a test to ensure that a runtime error is raised for `pow`'s inplace variant if resizing the base tensor is required during its invocation.
6. Added `float16` & `bfloat16` to `square`'s dtype lists in its `UnaryUfuncInfo`.
7. Removed redundant `dtypesIfCPU` and `dtypesIfCUDA` from `OpInfo`s where they are equal to `dtypes`.

Pull Request resolved: #55280

Reviewed By: jbschlosser

Differential Revision: D27591772

Pulled By: heitorschueroff

fbshipit-source-id: c7420811b32595bb3353149a61e54a73f2eb352b
krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
…orch#55280)

Summary:
#### Reason for relanding
Line 1607 of `torch/testing/_internal/common_methods_invocations.py` of pytorch#50999  had `dtype` instead of `dtype=torch.bool`, so 4 of the 9 sample inputs for `bool` had incorrect dtype. This bug was caught by pytorch#54949.

1. Added support for pow() on CPU for `float16` (`Half`) and `bfloat16` types.
Both `pow(Tensor, Scalar)` and `pow(Tensor, Tensor)` are now supported for the aforementioned types.
However autograd isn't supported for `Float16` on CPU yet, as `log_vml_cpu` can't be enabled for it.
2. heitorschueroff added `pow_tensor_scalar_optimized_kernel` to refactor & simplify `PowKernel.cpp`.
It provides a common path for all the complex types & floating point types (except Float16, due to lack of complete AVX2 vectorization support for it).  It replaced code that had previously been duplicated for (float, double) and complex types,
so PowKernel.cpp looks a lot cleaner now.
3. Enabled (unskipped) some tests for `erf`, `erfc`,`erfinv`, `tan` and `linalg.vector.norm` which were being skipped earlier due to `pow()` not having been implemented for `float16` & `bfloat16`.
4. Added an OpInfo for `pow()` & enabled some test cases for `pow()`.
5. Extended the coverage of existing tests for `pow` in `test_binary_ufuncs.py` in order to enable comparison with `numpy`, even with discontiguous tensors, and added a test to ensure that a runtime error is raised for `pow`'s inplace variant if resizing the base tensor is required during its invocation.
6. Added `float16` & `bfloat16` to `square`'s dtype lists in its `UnaryUfuncInfo`.
7. Removed redundant `dtypesIfCPU` and `dtypesIfCUDA` from `OpInfo`s where they are equal to `dtypes`.

Pull Request resolved: pytorch#55280

Reviewed By: jbschlosser

Differential Revision: D27591772

Pulled By: heitorschueroff

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

Labels

cla signed Merged open source Reverted 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.

10 participants