Implements cpu_kernel_multiple_outputs and torch.frexp by RockingJavaBean · Pull Request #51097 · pytorch/pytorch

RockingJavaBean · 2021-01-26T07:19:35Z

This PR implements the cpu_kernel_multiple_outputs to support returning multiple values in a CPU kernel.

auto iter = at::TensorIteratorConfig()
  .add_output(out1)
  .add_output(out2)
  .add_input(in1)
  .add_input(in2)
  .build();

at::native::cpu_kernel_multiple_outputs(iter,
  [=](float a, float b) -> std::tuple<float, float> {
    float add = a + b;
    float mul = a * b;
    return std::tuple<float, float>(add, mul);
  }
);

The out1 will equal to torch.add(in1, in2), while the result of out2 will be torch.mul(in1, in2).
It helps developers implement new torch functions that return two tensors more conveniently, such as NumPy-like functions divmod and frexp.

This PR adds torch.frexp function to exercise the new functionality provided by cpu_kernel_multiple_outputs.

facebook-github-bot · 2021-01-26T07:19:45Z

💊 CI failures summary and remediations

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

💚 💚 Looks good so far! There are no failures 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 to the (internal) Dr. CI Users group.

codecov · 2021-01-26T13:14:34Z

Codecov Report

Merging #51097 (c8198e1) into master (f6df18f) will increase coverage by 0.98%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master   #51097      +/-   ##
==========================================
+ Coverage   76.36%   77.35%   +0.98%     
==========================================
  Files        1886     1887       +1     
  Lines      184699   184804     +105     
==========================================
+ Hits       141048   142946    +1898     
+ Misses      43651    41858    -1793

…alues

mruberry · 2021-02-01T06:45:00Z

We should add a function that exercises this new functionality to test it, too. What about Numpy's frexp? (https://numpy.org/doc/stable/reference/generated/numpy.frexp.html).

…kernel_mutiple_outputs

RockingJavaBean · 2021-02-01T09:44:44Z

@mruberry Thanks for the kind suggestion, I will update this PR by adding torch.frexp using both cpu_kernel_multiple_outputs and gpu_kernel_multiple_outputs.

…iple_outputs.

…kernel_mutiple_outputs

heitorschueroff

Hi @RockingJavaBean , @mruberry and I took another look at your updates together. Overall, this PR is looking pretty good and close to ready. We really appreciate all the work you put into implementing frexp and also taking extra time to fix bugs in our test suite such as separating the float values.

There are still a few suggestion we have for your review:

We think we can reuse test_reference_numerics with a tweak, and while we appreciate you demonstrating that the other test_unary_ufunc.py tests can be adapted to work with frexp(), we're a little worried that change is too big. For the final PR, we'd like to suggest skipping those tests and leaving them unmodified.
For the test_frexp_out, add cases for incorrectly sized and noncontiguous inputs
Add supports_tensor_out=False to the UnaryUfuncInfo and fix the test_out_arg... to correctly query for the metadata instead of skipping the test.

We look forward to your next updates!

…kernel_mutiple_outputs

RockingJavaBean · 2021-03-11T05:56:23Z

I'm really grateful for the thorough review and invaluable suggestions throughout this PR.

This PR has been updated with the following changes:

update docs according to the suggestions, and add comments to the cpu_kernel_multiple_outputs related methods in Loops.h
revert the changes to test_ops.py, as well as the changes to existing tests in test_unary_ufuncs.py except for the tweak for test_reference_numerics.
add test cases for incorrectly sized and noncontiguous inputs to test_frexp_out, explicitly check exponent dtype, and compare it with NumPy counterpart in the customize test method test_frexp.

Please kindly take a look. @heitorschueroff @mruberry

heitorschueroff

@RockingJavaBean This last version looks ready, great work! Just one last change before I land this, it looks like the PR picked up some changes from another PR which I commented on, could you confirm this and fix it please, I'll land it then.

RockingJavaBean · 2021-03-11T15:16:17Z

@heitorschueroff I'm truly thankful for your kind review.
The above changes of torch.special_gammaln belongs to a merge commit fixing code conflicts, and would not be in the final changes of this PR, please kindly help confirm this.

heitorschueroff · 2021-03-11T18:05:32Z

@heitorschueroff I'm truly thankful for your kind review.
The above changes of torch.special_gammaln belongs to a merge commit fixing code conflicts, and would not be in the final changes of this PR, please kindly help confirm this.

You're correct. I'm landing the PR now, thank you for this great PR.

facebook-github-bot

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

mruberry

Nice job, @RockingJavaBean! This is a very technically complicated PR. I appreciate your thoughtfulness on both the technical challenges and the test architecture, too.

And thanks @heitorschueroff for reviewing this!

RockingJavaBean · 2021-03-12T15:18:51Z

It is my honor to contribute to the PyTorch project and it cannot be done without your generous help and guidance.
I'm really grateful to you. @mruberry @heitorschueroff

…kernel_mutiple_outputs

facebook-github-bot

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

facebook-github-bot · 2021-03-15T17:47:14Z

@heitorschueroff merged this pull request in da10ccd.

Summary: Close pytorch#51108 Related pytorch#38349 This PR implements the `cpu_kernel_multiple_outputs` to support returning multiple values in a CPU kernel. ```c++ auto iter = at::TensorIteratorConfig() .add_output(out1) .add_output(out2) .add_input(in1) .add_input(in2) .build(); at::native::cpu_kernel_multiple_outputs(iter, [=](float a, float b) -> std::tuple<float, float> { float add = a + b; float mul = a * b; return std::tuple<float, float>(add, mul); } ); ``` The `out1` will equal to `torch.add(in1, in2)`, while the result of `out2` will be `torch.mul(in1, in2)`. It helps developers implement new torch functions that return two tensors more conveniently, such as NumPy-like functions [divmod](https://numpy.org/doc/1.18/reference/generated/numpy.divmod.html?highlight=divmod#numpy.divmod) and [frexp](https://numpy.org/doc/stable/reference/generated/numpy.frexp.html#numpy.frexp). This PR adds `torch.frexp` function to exercise the new functionality provided by `cpu_kernel_multiple_outputs`. Pull Request resolved: pytorch#51097 Reviewed By: albanD Differential Revision: D26982619 Pulled By: heitorschueroff fbshipit-source-id: cb61c7f2c79873ab72ab5a61cbdb9203531ad469

facebook-github-bot added the cla signed label Jan 26, 2021

RockingJavaBean force-pushed the cpu_kernel_mutiple_outputs branch from 91dbe3f to c9a493a Compare January 26, 2021 09:23

RockingJavaBean changed the title ~~[WIP] implement cpu_kernel_multiple_outputs to support returning multiple values~~ implement cpu_kernel_multiple_outputs to support returning multiple values Jan 26, 2021

RockingJavaBean mentioned this pull request Jan 26, 2021

Function request: support returning multiple values in CPU kernel #51108

Closed

pytorchbot added the open source label Jan 26, 2021

heitorschueroff requested review from colesbury and heitorschueroff January 26, 2021 19:08

heitorschueroff added module: reductions module: TensorIterator triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Jan 26, 2021

implement cpu_kernel_multiple_outputs to support returning multiple v…

a76fe8d

…alues

RockingJavaBean force-pushed the cpu_kernel_mutiple_outputs branch from c9a493a to a76fe8d Compare January 27, 2021 05:25

mruberry requested review from mruberry and removed request for colesbury February 1, 2021 06:44

Merge branch 'master' of https://github.com/pytorch/pytorch into cpu_…

02cf0ce

…kernel_mutiple_outputs

RockingJavaBean added 8 commits February 4, 2021 11:22

add torch.frexp using cpu_kernel_multiple_outputs and gpu_kernel_mult…

dbc385b

…iple_outputs.

Merge branch 'master' of https://github.com/pytorch/pytorch into cpu_…

139b6b5

…kernel_mutiple_outputs

fix

0197e6d

CI fix and add multiple outputs support for UnaryUfuncInfo

9329cda

Merge branch 'master' of https://github.com/pytorch/pytorch into cpu_…

b66297b

…kernel_mutiple_outputs

fix flake8

ee3ef78

fix test_native_functions_yaml and try resolve ROCM issues

2cbc4a8

Merge branch 'master' of https://github.com/pytorch/pytorch into cpu_…

e66f752

…kernel_mutiple_outputs

RockingJavaBean requested review from albanD and soulitzer as code owners February 7, 2021 06:36

disable frexp on ROCM

4daccb2

mruberry reviewed Mar 2, 2021

View reviewed changes

Comment thread torch/_torch_docs.py Outdated

mruberry reviewed Mar 2, 2021

View reviewed changes

Comment thread torch/_torch_docs.py Outdated

heitorschueroff reviewed Mar 2, 2021

View reviewed changes

Merge branch 'master' of https://github.com/pytorch/pytorch into cpu_…

54472cc

…kernel_mutiple_outputs

RockingJavaBean force-pushed the cpu_kernel_mutiple_outputs branch from 123a787 to 95f662e Compare March 9, 2021 08:20

updates according to code reviews

52abf28

RockingJavaBean force-pushed the cpu_kernel_mutiple_outputs branch from 95f662e to 52abf28 Compare March 9, 2021 11:14

RockingJavaBean added 3 commits March 10, 2021 16:16

add comments according to review

b20c25a

Merge branch 'master' of https://github.com/pytorch/pytorch into cpu_…

f74b527

…kernel_mutiple_outputs

updates

742a99f

RockingJavaBean force-pushed the cpu_kernel_mutiple_outputs branch from de5c23e to 742a99f Compare March 10, 2021 12:30

heitorschueroff self-requested a review March 10, 2021 15:31

improve test

f34c685

RockingJavaBean requested a review from mruberry March 11, 2021 05:56

heitorschueroff reviewed Mar 11, 2021

View reviewed changes

Comment thread aten/src/ATen/native/UnaryOps.cpp

heitorschueroff approved these changes Mar 11, 2021

View reviewed changes

facebook-github-bot reviewed Mar 11, 2021

View reviewed changes

mruberry approved these changes Mar 12, 2021

View reviewed changes

Merge branch 'master' of https://github.com/pytorch/pytorch into cpu_…

c8198e1

…kernel_mutiple_outputs

facebook-github-bot reviewed Mar 15, 2021

View reviewed changes

facebook-github-bot closed this in da10ccd Mar 15, 2021

facebook-github-bot added the Merged label Mar 15, 2021

RockingJavaBean deleted the cpu_kernel_mutiple_outputs branch March 16, 2021 01:07

Conversation

RockingJavaBean commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

codecov Bot commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mruberry commented Feb 1, 2021

Uh oh!

RockingJavaBean commented Feb 1, 2021

Uh oh!

Uh oh!

Uh oh!

heitorschueroff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RockingJavaBean commented Mar 11, 2021

Uh oh!

Uh oh!

heitorschueroff left a comment

Choose a reason for hiding this comment

Uh oh!

RockingJavaBean commented Mar 11, 2021

Uh oh!

heitorschueroff commented Mar 11, 2021

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

RockingJavaBean commented Mar 12, 2021

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RockingJavaBean commented Jan 26, 2021 •

edited

Loading

facebook-github-bot commented Jan 26, 2021 •

edited

Loading

codecov Bot commented Jan 26, 2021 •

edited

Loading