[numpy] Add `torch.exp2` #44184

kshitij12345 · 2020-09-04T08:20:23Z

Reference #42515

TODO

Add tests
Add docs

dr-ci · 2020-09-04T08:44:17Z

💊 CI failures summary and remediations

As of commit fd02fb7 (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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 80 times.

* fix incorrect type dispatch in cuda. * remove bfloat16 support.

kshitij12345 · 2020-09-04T12:13:39Z

@mruberry Please review :)

kshitij12345 · 2020-09-04T14:46:30Z

Not sure why the JIT tests are failing as they run successfully on local.

aten/src/ATen/native/cpu/UnaryOpsKernel.cpp

aten/src/ATen/native/cuda/UnaryOpsKernel.cu

aten/src/ATen/native/native_functions.yaml

mruberry · 2020-09-10T04:34:18Z

test/test_torch.py

        b = torch.exp(torch.ones(1, dtype=dtype, device=device))
        self.assertEqual(a, b.expand(2 ** 31))

+    def _unary_op_vs_numpy_template(self, torch_fn, np_fn, device, dtype, with_extremal):


exp is currently tested in TestTorchMathOps, but that's being refactored into a new testing mechanism that uses "OpInfos". Take a look at how unary ufuncs like exp2 are registered here:

pytorch/torch/testing/_internal/common_methods_invocations.py

Line 203 in 6324ef4

UnaryUfuncInfo('acos',

You can register exp2 the same way and it will be tested automatically. You'll need to fill out the ref field and set the dtypesIfCPU and dtypesIfCUDA fields since exp2 doesn't support complex values.

The OpInfo method of testing is new and I would appreciate your feedback trying to use it. I hope it's straightforward.

Should I remove this test as the PR will update the UnaryUfuncInfo Operator list. Or both existing would fine. ( I think removal makes more sense, as both do similar testing)

Yep. It should be redundant with the OpInfo-based tests (in test_unary_ufuncs.py). Better remove it.

Had to do

UnaryUfuncInfo('exp2', ref=np.exp2, dtypes=floating_types_and(torch.half), dtypesIfCPU=None, # Had to explicitly set to None dtypesIfCUDA=None, dtypesIfROCM=None,), # Had to explicitly set to None

One would expect that if only dtypes is given all devices will verify only those dtypes based on the code below

pytorch/torch/testing/_internal/common_methods_invocations.py

Lines 73 to 76 in 63ca5e0

self.dtypes = dtypes

self.dtypesIfCPU = dtypesIfCPU if dtypesIfCPU is not None else dtypes

self.dtypesIfCUDA = dtypesIfCUDA if dtypesIfCUDA is not None else dtypes

self.dtypesIfROCM = dtypesIfROCM if dtypesIfROCM is not None else dtypes

But with the following defaults in constructor,

pytorch/torch/testing/_internal/common_methods_invocations.py

Lines 160 to 167 in 63ca5e0

def __init__(self,

name, # the string name of the function

*,

ref, # a reference function

dtypes=floating_types(),

dtypesIfCPU=floating_and_complex_types_and(torch.bfloat16),

dtypesIfCUDA=floating_and_complex_types_and(torch.half),

dtypesIfROCM=floating_types_and(torch.half),

That behaviour is not honored. I think the default value for those arguments should be None.

You're right that you have to explicitly override because of the defaults. We could add a more complex inference system for the default dtypes where the defaults are None but None is interpreted as first checking dtypes, if set, and then reverting to its current default value. Let's see if it comes up again?

torch/_torch_docs.py

mruberry · 2020-09-10T04:41:28Z

torch/testing/_internal/common_methods_invocations.py

        ('expand_as', (S, 1, 1), (torch.rand(S, S, S),), '', (False,)),
        ('exp', (S, S, S), NO_ARGS, '', (True,)),
        ('exp', (), NO_ARGS, 'scalar', (True,)),
+        ('exp2', (S, S, S), NO_ARGS, '', (True,)),


You can leave these entries for now. Although they'll be mostly redundant with the OpInfo entry there's still a couple tests that only use method_tests.

mruberry

Hey @kshitij12345! This is good work, as always. I made a few suggestions, including trying the new OpInfo-based test pattern. I'm very curious to hear how easy/hard it is for you to use it.

codecov · 2020-09-10T09:11:47Z

Codecov Report

Merging #44184 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #44184   +/-   ##
=======================================
  Coverage   67.98%   67.98%           
=======================================
  Files         384      384           
  Lines       49586    49589    +3     
=======================================
+ Hits        33712    33715    +3     
  Misses      15874    15874

Impacted Files	Coverage Δ
torch/overrides.py	`98.05% <ø> (ø)`
torch/_tensor_docs.py	`100.00% <100.00%> (ø)`
torch/_torch_docs.py	`100.00% <100.00%> (ø)`
...ch/testing/_internal/common_methods_invocations.py	`92.17% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c58a01...fd02fb7. Read the comment docs.

kshitij12345 · 2020-09-11T11:02:06Z

@mruberry

Thanks for the help. Have updated the assertion and those errors are gone. However the previous error is still there and showing up.

https://app.circleci.com/pipelines/github/pytorch/pytorch/213001/workflows/9aeec75e-b846-4a51-9ef8-d39e7afac17a/jobs/7412397

I had previously tried to run it locally. But couldn't reproduce the failure. Please take a look :).
Thank You!

mruberry · 2020-09-11T11:22:10Z

Would you try changing this line (and the other method test line(s)) from:

('exp2', (S, S, S), NO_ARGS, '', (True,)),

to

('exp2', (S, S, S), NO_ARGS, '', (False,)),

or

('exp2', (S, S, S), NO_ARGS, ''),

I think that will fix this issue. What I think is happening is that the check is trying to fuse exp2 and verify it's fused, but it doesn't know how to do that because exp2 is a new function. If that doesn't work I recommend deleting the method_test entries to resolve this issue.

kshitij12345 · 2020-09-11T11:31:36Z

I think that will fix this issue. What I think is happening is that the check is trying to fuse exp2 and verify it's fused, but it doesn't know how to do that because exp2 is a new function. If that doesn't work I recommend deleting the method_test entries to resolve this issue.

Thanks for the clarification. That makes sense! Will try that!

Any idea as to how it would work on my local system? 🤔

kshitij12345 · 2020-09-11T13:25:21Z

@mruberry Thanks. Looks like those tests are passing now. Would be great if you can have another look. I think the PR is almost done.

mruberry · 2020-09-13T12:43:11Z

I think that will fix this issue. What I think is happening is that the check is trying to fuse exp2 and verify it's fused, but it doesn't know how to do that because exp2 is a new function. If that doesn't work I recommend deleting the method_test entries to resolve this issue.

Thanks for the clarification. That makes sense! Will try that!

Any idea as to how it would work on my local system? 🤔

It's because the fusion is only enabled on some builds. I think it's only Windows builds.

aten/src/ATen/native/cuda/UnaryOpsKernel.cu

test/test_torch.py

mruberry

Nice work, @kshitij12345! Thank you for trying out our new OpInfo test method!

I made a few minor comments (eliminate an extra semicolon, delete a redundant test) but this looks great. Thanks for another awesome PR! Just ping me when the minor cleanup is done.

* remove redundant test * remove stray semi-colon

kshitij12345 · 2020-09-13T14:28:26Z

@mruberry

It's because the fusion is only enabled on some builds. I think it's only Windows builds.

Ah. I see. Thanks!

Also have addressed the latest comments.

facebook-github-bot

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

mruberry · 2020-09-14T04:11:27Z

Internal tests are failing because exp2 doesn't appear to be available in the standard namespace on android builds:

aten/src/ATen/native/cpu/UnaryOpsKernel.cpp:356:51: error: no member named 'exp2' in namespace 'std'
        [=](scalar_t a) -> scalar_t { return std::exp2(a); });

cc @dreiss

We've had this issue in the past with @muthuArivoli's PRs, here's the fix we identified:

#42291 (comment)

Basically a couple stubs need to be added.

Let me know if you have additional questions about this, @kshitij12345. Once the stubs are added I can re-import this PR and run the Android tests again.

kshitij12345 · 2020-09-14T05:07:03Z

@mruberry Thanks for the update.

I don't think we need to update the vectorized neon implementations as we don't use the vector implementations anywhere currently. Is that right?

So only place we might need to update is pytorch/c10/util/math_compat.h

mruberry · 2020-09-14T05:08:11Z

@mruberry Thanks for the update.

I don't think we need to update the vectorized neon implementations as we don't use the vector implementations anywhere currently. Is that right?

So only place we might need to update is pytorch/c10/util/math_compat.h

That's probably true. Let's try that first.

kshitij12345 · 2020-09-14T05:12:23Z

@mruberry Have updated the pytorch/c10/util/math_compat.h.

facebook-github-bot

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

facebook-github-bot · 2020-09-14T12:10:53Z

@mruberry merged this pull request in c68a99b.

mruberry · 2020-09-14T16:30:00Z

Looks like that did it. Thanks @kshitij12345!

Summary: Reference #42515 TODO * [x] Add tests * [x] Add docs Pull Request resolved: #44184 Reviewed By: ngimel Differential Revision: D23674237 Pulled By: mruberry fbshipit-source-id: 7f4fb1900fad3051cd7fc9d3d7f6d985c5fb093c

kshitij12345 added 2 commits September 4, 2020 13:46

add exp2 support

90646ca

Merge branch 'master' into develop/numpy/exp2

4318fbe

pytorchbot added the open source label Sep 4, 2020

kshitij12345 marked this pull request as draft September 4, 2020 08:33

kshitij12345 added 5 commits September 4, 2020 15:33

update docs

8517e31

add to interned_strings

97607f1

changes

c0b4788

* fix incorrect type dispatch in cuda. * remove bfloat16 support.

add test

4697263

add autograd support

b4c3b44

kshitij12345 marked this pull request as ready for review September 4, 2020 12:13

kshitij12345 added 2 commits September 4, 2020 19:16

add to overrides

0f83ebf

fix copy pasted docs

ff1bedc

Merge branch 'master' into develop/numpy/exp2

2de12b4

mruberry mentioned this pull request Sep 5, 2020

Unary Ufunc tracking issue #42515

Closed

8 tasks

hameerabbasi requested a review from mruberry September 7, 2020 07:25