Alias for logsumexp to special namespace #58838

krshrimali · 2021-05-24T06:59:41Z

See #50345

cc: @kshitij12345 @lezcano @mruberry

facebook-github-bot · 2021-05-24T06:59:47Z

💊 CI failures summary and remediations

As of commit 7de4af3 (more details on the Dr. CI page and at hud.pytorch.org/pr/58838):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

Preview docs built from this PR

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.

Click here to manually regenerate this comment.

aten/src/ATen/native/native_functions.yaml

torch/overrides.py

torch/special/__init__.py

torch/testing/_internal/common_methods_invocations.py

Hard code inputs

krshrimali · 2021-06-08T09:33:00Z

Updates:

Apologies for a long hold on this PR. Most of the issues have been resolved, waiting for the tests to pass then it will be ready for review. :)

kshitij12345

Overall looks good to me. But one suggested change.

Thanks @krshrimali

torch/csrc/api/include/torch/special.h

test/test_ops.py

lezcano

Left a few small points. Overall looks very good, thanks @krshrimali

aten/src/ATen/native/ReduceOps.cpp

aten/src/ATen/native/native_functions.yaml

test/test_ops.py

torch/special/__init__.py

lezcano · 2021-06-08T11:05:05Z

torch/special/__init__.py

+Example::
+
+    >>> a = torch.randn(3, 3)
+    >>> torch.logsumexp(a, 1)


This should be torch.special.logsumexp.

It would also be more meaningful to write here:

>>> torch.dist(torch.special.logsumexp(a, 1), torch.log(torch.sum(torch.exp(a), dim)))
tensor(1.1921e-07)

I'm unfortunately on a different opinion with this change, I'm thinking that we should be consistent with examples on other functions and I feel having >>> torch.special.logsumexp(a, 1) is OK here.

Okay, I think your suggestion also makes sense, so to satisfy both simplicity as well as give a better meaning to what the function does. How about this instead?

>>> torch.special.logsumexp(a, 1) #output >>> torch.dist(torch.special.logsumexp(a, 1), torch.log(torch.sum(torch.exp(a), dim))) #output

I think having both would be a good addition. Note that the formula I wrote had an errata, and dim should be 1.

krshrimali · 2021-06-22T08:31:52Z

Gentle ping, @mruberry - Can you please take a look at this whenever you find time? Thanks!

lezcano

I do not know enough to be able to say whether the changes in the JIT are fine, I'll let @mruberry assess that or find the person that knows about this. Everything else looks good to me. Thank you Kush!

On this topic, I have a question. Would this fix happen to fix all (or a number of) those tests that we used to ignore in the OpInfos, or does it address something different? @krshrimali @kshitij12345
If it is so, this is great! We should then remove the relevan skips.
If it wasn't, how do other functions that have this problem deal with the JIT errors that we found here?

krshrimali · 2021-06-22T09:32:09Z

I do not know enough to be able to say whether the changes in the JIT are fine, I'll let @mruberry assess that or find the person that knows about this. Everything else looks good to me. Thank you Kush!

Thanks, @lezcano for taking a closer look at this PR. Appreciate your comments. Will update the doc to add both formulae.

On this topic, I have a question. Would this fix happen to fix all (or a number of) those tests that we used to ignore in the OpInfos, or does it address something different? @krshrimali @kshitij12345

This is a great question, the fix in this PR is for test_jit_alias_remapping (that is only for JIT remapping tests on aliases). I just noticed that there are a few tests which are skipped, and which probably can be solved by the fix from this PR. I'll see if they pass on this branch.

If it is so, this is great! We should then remove the relevan skips.

Agreed! :)

krshrimali · 2021-06-22T10:41:33Z

Update:

There are currently 2 tests skipped:

vstack which is because of the function taking sequence of tensors as input instead of just a tensor. This is not fixed in this PR but again we can remove i0 from the formals and directly pass sample inputs to actuals, but I am not too sure if this is the correct solution for this issue.
movedim - this has been fixed by this PR. :)

Thanks for asking the right question, @lezcano!

cc: @mruberry @lezcano @kshitij12345

codecov · 2021-06-22T16:54:43Z

Codecov Report

Merging #58838 (89352af) into master (45ae2e7) will increase coverage by 0.00%.
The diff coverage is 66.66%.

❗ Current head 89352af differs from pull request most recent head 2f7d050. Consider uploading reports for the commit 2f7d050 to get more accurate results

@@           Coverage Diff           @@
##           master   #58838   +/-   ##
=======================================
  Coverage   76.23%   76.24%           
=======================================
  Files        2054     2054           
  Lines      205033   205117   +84     
=======================================
+ Hits       156306   156387   +81     
- Misses      48727    48730    +3

mruberry · 2021-06-24T04:15:07Z

torch/_torch_docs.py

-    >>> a = torch.randn(3, 3)
-    >>> torch.logsumexp(a, 1)
-    tensor([ 0.8442,  1.4322,  0.8711])
+Alias for :func:`torch.special.logsumexp`.


Let's keep the docs in torch for now and have special.logsumexp alias it.

mruberry · 2021-06-24T04:15:27Z

torch/testing/_internal/common_methods_invocations.py

-           skips=(
-               # Expected a value of type 'int' for argument 'source'
-               #   but instead found type 'list'.
-               SkipInfo('TestJit', 'test_jit_alias_remapping'),


Awesome skip removal

mruberry

This looks really good. cc @eellison, who's interested in reducing jit test skips.

@eellison, do these jit test changes make sense?

I also made one comment about keeping the docs on torch.logsumexp for now (we'll review what goes in special vs. torch later)

krshrimali · 2021-06-29T05:31:36Z

Hi, @eellison and @mruberry - Gentle ping if this can be looked at. Thanks!

eellison

Thanks for removing the skipped test 😍 😍 😍 LGTM if tests pass

…o logsumexp

mruberry

LGTM! Thanks @krshrimali

facebook-github-bot · 2021-07-06T20:12:07Z

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

facebook-github-bot · 2021-07-07T20:34:40Z

@mruberry merged this pull request in 423523d.

need to test, logsumexp for special, init not added

774a1fb

krshrimali requested a review from ezyang as a code owner May 24, 2021 06:59

facebook-github-bot added the cla signed label May 24, 2021

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label May 24, 2021

krshrimali changed the title ~~[WIP] Alias for logsumexp to special namespace~~ (WIP) Alias for logsumexp to special namespace May 24, 2021

krshrimali marked this pull request as draft May 24, 2021 07:00

pytorchbot added the open source label May 24, 2021

krshrimali added the module: special Functions with no exact solutions, analogous to those in scipy.special label May 24, 2021

krshrimali mentioned this pull request May 24, 2021

torch.special tracking issue #50345

Open

17 tasks

krshrimali added 4 commits May 24, 2021 05:13

Fix minor typos, add doc for special

25b992c

Minor fix, pass result first

46f4adc

import multi_dim_common

60553f9

Tabs to spaces, linter

c79619d

ezyang removed their request for review May 24, 2021 18:52

kshitij12345 reviewed May 25, 2021

View reviewed changes

aten/src/ATen/native/native_functions.yaml Outdated Show resolved Hide resolved

kshitij12345 reviewed May 25, 2021

View reviewed changes

torch/overrides.py Show resolved Hide resolved

kshitij12345 reviewed May 25, 2021

View reviewed changes

torch/special/__init__.py Show resolved Hide resolved

kshitij12345 reviewed May 26, 2021

View reviewed changes

torch/testing/_internal/common_methods_invocations.py Show resolved Hide resolved

krshrimali added 4 commits June 8, 2021 11:29

Doc fixes, debugging...

d709e23

Fixing alias_remapping function for signatures

6a8a3e6

Fix alias_remapping function for signatures

3c420ac

Hard code inputs

Merge branch 'master' into logsumexp

18b5b00

kshitij12345 reviewed Jun 8, 2021

View reviewed changes

torch/csrc/api/include/torch/special.h Outdated Show resolved Hide resolved

test/test_ops.py Outdated Show resolved Hide resolved

lezcano reviewed Jun 8, 2021

View reviewed changes

krshrimali changed the title ~~(WIP) Alias for logsumexp to special namespace~~ Alias for logsumexp to special namespace Jun 8, 2021

krshrimali added 3 commits June 9, 2021 09:39

Remove args arg for method/inplace variants

4f9fbbd

Import tensor from torch to avoid undefined value for tensors

21d63ac

Merge remote-tracking branch 'upstream/master' into logsumexp

9a59584

krshrimali added 5 commits June 21, 2021 09:35

Merge remote-tracking branch 'upstream/master' into logsumexp

a4e90ba

Minor fix, tests should pass

8aeb49d

Merge branch 'master' into logsumexp

f3a5e05

Review: add comment for args_kw

90c1e2a

Merge branch 'logsumexp' of github.com:krshrimali/pytorch into logsumexp

08c715f

lezcano reviewed Jun 22, 2021

View reviewed changes

krshrimali added 2 commits June 22, 2021 10:08

Remove skip for movedim (alias jit remapping test)

e6b5022

Update doc with meaningful formulae

89352af

mruberry reviewed Jun 24, 2021

View reviewed changes

krshrimali added 2 commits June 24, 2021 06:11

Keep docs in torch for now, let special alias it - as per review

2f7d050

Merge remote-tracking branch 'upstream/master' into logsumexp

8d50918

eellison approved these changes Jun 30, 2021

View reviewed changes

krshrimali added 4 commits July 1, 2021 12:00

Merge remote-tracking branch 'upstream/master' into logsumexp

ec010a4

Merge remote-tracking branch 'upstream/master' into logsumexp

1373509

Revise doc, special use removed in torch.logsumexp

27d6a88

Merge branch 'logsumexp' of https://github.com/krshrimali/pytorch int…

7de4af3

…o logsumexp

mruberry approved these changes Jul 6, 2021

View reviewed changes

facebook-github-bot closed this in 423523d Jul 7, 2021

facebook-github-bot added the Merged label Jul 7, 2021

AnirudhDagar mentioned this pull request Aug 7, 2021

Support torch.concat alias, add cat OpInfo & remove OpInfo test_out skips {cat, stack, hstack, vtack, dstack} #62560

Closed

9 tasks

Alias for logsumexp to special namespace #58838

Alias for logsumexp to special namespace #58838

Uh oh!

Conversation

krshrimali commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krshrimali commented Jun 8, 2021

Uh oh!

kshitij12345 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lezcano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lezcano Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krshrimali Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

lezcano Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krshrimali commented Jun 22, 2021

Uh oh!

lezcano left a comment

Choose a reason for hiding this comment

Uh oh!

krshrimali commented Jun 22, 2021

Uh oh!

krshrimali commented Jun 22, 2021

Uh oh!

codecov bot commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mruberry Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

mruberry Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

krshrimali commented Jun 29, 2021

Uh oh!

eellison 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!

facebook-github-bot commented Jul 6, 2021

Uh oh!

facebook-github-bot commented Jul 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

krshrimali commented May 24, 2021 •

edited

Loading

facebook-github-bot commented May 24, 2021 •

edited

Loading

lezcano Jun 8, 2021 •

edited

Loading

lezcano Jun 22, 2021 •

edited

Loading

codecov bot commented Jun 22, 2021 •

edited

Loading