OpInfo: `diag_embed`, `diagonal` #58642

krshrimali · 2021-05-20T04:54:41Z

See: #54261.

…ssfully

facebook-github-bot · 2021-05-20T04:54:47Z

💊 CI failures summary and remediations

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

2/2 failures possibly* introduced in this PR
- 1/2 non-scanned failure(s)

1 failure not recognized by patterns:

Job	Step	Action
^{pytorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_test2}	^{Run tests}	🔁 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.

Click here to manually regenerate this comment.

krshrimali · 2021-05-20T04:55:17Z

torch/testing/_internal/common_methods_invocations.py

+    make_arg = partial(make_tensor, dtype=dtype, device=device, requires_grad=requires_grad)
+
+    cases = ((S, S),)
+
+    def generator():
+        for shape in cases:
+            yield(SampleInput(make_arg(shape)))
+
+    return list(generator())


Keeping it in this format to maintain consistency with all the OpInfos. In case there is any addition required to the cases, this keeps it flexible.

kshitij12345

Nice!

However looking at the docs and the signature of torch.diag_embed, we should probably add more cases which cover all arguments for the function (to improve the coverage). What do you think about it?

Thanks!!

cc: @mruberry what do you think?

Docs: https://pytorch.org/docs/stable/generated/torch.diag_embed.html

mruberry · 2021-05-20T06:56:46Z

Nice!

However looking at the docs and the signature of torch.diag_embed, we should probably add more cases which cover all arguments for the function (to improve the coverage). What do you think about it?

Thanks!!

cc: @mruberry what do you think?

Extending the sample inputs to test offset, dim1, and dim2 sounds like a great idea

krshrimali · 2021-05-20T07:03:29Z

Updating this PR's scope after discussing with @kshitij12345, to try and merge sample inputs of diag_embed with diagonal. Also taking a look if diag can be merged as well, keeping the functions neat & clean. Will remove the WIP tag once all tests pass + the solution is ready to be reviewed.

krshrimali · 2021-05-20T11:12:49Z

Hi, @kshitij12345 - this is ready for review. There are a couple tests failing, which I think should be resolved after merging with master + submodule update.

Also, I decided not to merge sample_inputs_diag with sample_inputs_diagonal (for diagonal, diag_embed) because diag requires vec_sample = SampleInput(make_tensor((M, ), ...)) as an input sample which isn't accepted for diagonal, diag_embed. We can still do something like this though:

def sample_inputs_diagonal_functions(op_info, device, dtype, requires_grad, limit_to_2d=False, supports_vec=True, **kwargs):
    make_arg = partial(make_tensor, dtype=dtype, device=device, requires_grad=requires_grad, low=None, high=None)
     
    tensors_2d = ( ... 2d tensors ... )
    tensors_3d = ( ... 3d tensors ... )
    args_2d = ( ... args 2d ... )
    args_3d = ( ... args 3d ... ) 
   
    if limit_to_2d:
        # True for diag
        tensors = [*product(tensors_2d, args_2d)]
        samples = [SampleInput(tensor, args=arg) for tensor, arg in tensors]
    else:
        # For diagonal, diag_embed
        tensors = [*product(tensors_2d, args_2d), *product(tensors_3d, args_3d)]
        samples = [SampleInput(tensor, args=arg) for tensor, arg in tensors]
    if supports_vec:
        # For diag, diag_embed
        return samples + [SampleInput(make_arg((M, )))]
    return samples

But I'm not sure how neat is this. Note that:

diag only takes tensors till 2D dim. (it only accepts matrix or a vector).
diag_embed accepts vector input. (accepts any dim input)
diagonal doesn't accept vector input. (accepts any dim input > 1)

>>> torch.diag(torch.randn(1,))
tensor([[-0.9189]])
>>> torch.diag_embed(torch.randn(1,))
tensor([[-0.5603]])
>>> torch.diagonal(torch.randn(1,))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: Dimension out of range (expected to be in range of [-1, 0], but got 1)

krshrimali · 2021-05-20T11:33:24Z

Hi, @kshitij12345 - this is ready for review. There are a couple tests failing, which I think should be resolved after merging with master + submodule update.

Also, I decided not to merge sample_inputs_diag with sample_inputs_diagonal (for diagonal, diag_embed) because diag requires vec_sample = SampleInput(make_tensor((M, ), ...)) as an input sample which isn't accepted for diagonal, diag_embed. We can still do something like this though:

def sample_inputs_diagonal_functions(op_info, device, dtype, requires_grad, limit_to_2d=False, supports_vec=True, **kwargs):
    make_arg = partial(make_tensor, dtype=dtype, device=device, requires_grad=requires_grad, low=None, high=None)
     
    tensors_2d = ( ... 2d tensors ... )
    tensors_3d = ( ... 3d tensors ... )
    args_2d = ( ... args 2d ... )
    args_3d = ( ... args 3d ... ) 
   
    if limit_to_2d:
        # True for diag
        tensors = [*product(tensors_2d, args_2d)]
        samples = [SampleInput(tensor, args=arg) for tensor, arg in tensors]
    else:
        # For diagonal, diag_embed
        tensors = [*product(tensors_2d, args_2d), *product(tensors_3d, args_3d)]
        samples = [SampleInput(tensor, args=arg) for tensor, arg in tensors]
    if supports_vec:
        # For diag, diag_embed
        return samples + [SampleInput(make_arg((M, )))]
    return samples

But I'm not sure how neat is this. Note that:

diag only takes tensors till 2D dim. (it only accepts matrix or a vector).
diag_embed accepts vector input. (accepts any dim input)
diagonal doesn't accept vector input. (accepts any dim input > 1)

>>> torch.diag(torch.randn(1,))
tensor([[-0.9189]])
>>> torch.diag_embed(torch.randn(1,))
tensor([[-0.5603]])
>>> torch.diagonal(torch.randn(1,))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: Dimension out of range (expected to be in range of [-1, 0], but got 1)

On re-thinking, it might just be a good idea to merge all the sample inputs into one. It's okay to add more args as long as their names make sense, what do you think @kshitij12345 @mruberry ?

Here is how the sample_inputs func for diag, diag_embed, diagonal can look like:

def sample_inputs_diagonal_functions(op_info, device, dtype, requires_grad, limit_to_2d=False, supports_vec=True, **kwargs):
    make_arg = partial(make_tensor, dtype=dtype, device=device, requires_grad=requires_grad, low=None, high=None)

    # 2D Tensors
    tensors_2d = (
        make_arg((M, M), low=None, high=None),
        make_arg((3, 5), low=None, high=None),
        make_arg((5, 3), low=None, high=None),
    )

    # 3D Tensors
    tensors_3d = (
        make_arg((M, M, M), low=None, high=None),
    )

    args_2d = ((), (2,), (-2,), (1,), (2,))
    args_3d = ((1, 1, 2), (2, 0, 1), (-2, 0, 1))

    if limit_to_2d:
        # True for diag
        tensors = [*product(tensors_2d, args_2d)]
        samples = [SampleInput(tensor, args=arg) for tensor, arg in tensors]
    else:
        # For diagonal, diag_embed
        tensors = [*product(tensors_2d, args_2d), *product(tensors_3d, args_3d)]
        samples = [SampleInput(tensor, args=arg) for tensor, arg in tensors]
    if supports_vec:
        # For diag, diag_embed
        return samples + [SampleInput(make_arg((M, )))]
    return samples

kshitij12345

Looks good overall. Left a few comments!

Thanks!!

kshitij12345 · 2021-05-20T11:27:36Z

torch/testing/_internal/common_methods_invocations.py

@@ -2913,6 +2913,28 @@ def sample_inputs_diag(op_info, device, dtype, requires_grad, **kwargs):

    return samples + [vec_sample]

+def sample_inputs_diagonal(op_info, device, dtype, requires_grad, **kwargs):
+    make_arg = partial(make_tensor, dtype=dtype, device=device, requires_grad=requires_grad, low=None, high=None)


pytorch/torch/testing/_internal/common_utils.py

Line 1725 in 5caccbe

def make_tensor(size, device: torch.device, dtype: torch.dtype, *, low=None, high=None,

You can skip passing low and high

Yes, done, thanks!

kshitij12345 · 2021-05-20T11:27:51Z

torch/testing/_internal/common_methods_invocations.py

+
+    # 2D Tensors
+    tensors_2d = (
+        make_arg((M, M), low=None, high=None),


You can skip passing low and high

Done, thanks!

kshitij12345 · 2021-05-20T11:35:30Z

torch/testing/_internal/common_methods_invocations.py

+    args_2d = ((), (2,), (-2,), (1,), (2,))
+    args_3d = ((1, 1, 2), (2, 0, 1), (-2, 0, 1))
+
+    tensors = [*product(tensors_2d, args_2d), *product(tensors_3d, args_3d)]


I think using product with already materialised tensor is not good.

I tried without the clone_inputs fuction in the patch. But it looks to be necessary as the sample_inputs for tile/repeat uses itertools.product

Ref: #52135 (comment)

So I'd suggest taking product of shape_2d and args_2d and then iterating of the product and materializing unique tensor for each arg (like we do usually).

Not sure if it isn't an issue anymore.
NOTE: This will lead to more tensors being materialised than the current approach.
cc: @mruberry

We should prioritize each sample input having an independent set of tensors that, if modified, won't affect other sample inputs. So I agree that taking the product over the non-tensor constructor args is preferable, even though it will generate more tensors. Reusing tensors in the test suite is a challenge I don't think we want (at least not now).

This should be resolved in recent commit.

kshitij12345 · 2021-05-20T11:37:07Z

torch/testing/_internal/common_methods_invocations.py

+           sample_inputs_func=sample_inputs_diagonal),
+    OpInfo('diagonal',
+           dtypes=all_types_and_complex_and(torch.bool, torch.bfloat16, torch.float16),
+           supports_out=False,


Surprise! I thought method_tests always had entries for operators which supported autograd.

I wonder what they do for operators which don't support autograd.

cc: @mruberry

Where do you see no autograd support?

Ouch! I saw it as support_autograd. My bad 😅

kshitij12345 · 2021-05-20T11:38:01Z

torch/testing/_internal/common_methods_invocations.py

+        make_arg((M, M, M), low=None, high=None),
+    )
+
+    args_2d = ((), (2,), (-2,), (1,), (2,))


2 is repeated here.

Sorry, should be fixed in the recent commit.

kshitij12345 · 2021-05-20T11:39:51Z

Here is how the sample_inputs func for diag, diag_embed, diagonal can look like:

I think it is better to seperate diag as the operator looks to be fairly different from diag_embed and diagonal to me.

kshitij12345 · 2021-05-20T11:46:19Z

@krshrimali
diag already has an OpInfo (so no need to worry about the same).

pytorch/torch/testing/_internal/common_methods_invocations.py

Lines 4398 to 4402 in 5caccbe

    
           OpInfo('diag', 
        
                  dtypes=all_types_and_complex_and(torch.bool), 
        
                  dtypesIfCPU=all_types_and_complex_and(torch.bool), 
        
                  dtypesIfCUDA=all_types_and_complex_and(torch.bool, torch.half, torch.bfloat16), 
        
                  sample_inputs_func=sample_inputs_diag),

krshrimali · 2021-05-20T11:48:01Z

@krshrimali
diag already has an OpInfo (so no need to worry about the same).

pytorch/torch/testing/_internal/common_methods_invocations.py

Lines 4398 to 4402 in 5caccbe

OpInfo('diag',

dtypes=all_types_and_complex_and(torch.bool),

dtypesIfCPU=all_types_and_complex_and(torch.bool),

dtypesIfCUDA=all_types_and_complex_and(torch.bool, torch.half, torch.bfloat16),

sample_inputs_func=sample_inputs_diag),

Yes, thanks @kshitij12345. I'm aware of this, was just thinking if we should merge the sample_inputs functions for all 3. Looks like it's okay for them to be separate, as per your inputs. :)

kshitij12345

LGTM!
Have a made a few minor nits.

Thanks!

kshitij12345 · 2021-05-20T12:29:02Z

torch/testing/_internal/common_methods_invocations.py

-    return samples
+    def generator():
+        for shapes, args in zip([shapes_2d, shapes_3d], [args_2d, args_3d]):
+            for shape, arg in product(shapes, args):


minor:
How about

for shape, arg in chain(product(shapes_2d, args_2d), product(shapes_3d, args_3d)):

kshitij12345 · 2021-05-20T12:39:57Z

torch/testing/_internal/common_methods_invocations.py

@@ -2913,6 +2913,25 @@ def sample_inputs_diag(op_info, device, dtype, requires_grad, **kwargs):

    return samples + [vec_sample]

+def sample_inputs_diagonal(op_info, device, dtype, requires_grad, **kwargs):


minor nit: sample_inputs_diagonal_diag_embed

Makes sense! :)

mruberry

Nice work, @krshrimali and thank you for reviewing @kshitij12345!

This just needs a rebase; ping me when the tests are passing, @krshrimali

facebook-github-bot · 2021-05-23T04:54:06Z

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

facebook-github-bot · 2021-05-24T08:03:10Z

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

krshrimali · 2021-05-25T04:07:43Z

Nice work, @krshrimali and thank you for reviewing @kshitij12345!

This just needs a rebase; ping me when the tests are passing, @krshrimali

Thanks, @mruberry! Not sure if the failing test is because of this PR. test_nccl_high_priority_stream is failing, and doesn't look relevant to this PR. Do you think this PR is ready to be merged?

facebook-github-bot · 2021-05-25T18:54:37Z

@mruberry merged this pull request in b9d1ad9.

Summary: See: pytorch#54261. Pull Request resolved: pytorch#58642 Reviewed By: ngimel Differential Revision: D28627226 Pulled By: mruberry fbshipit-source-id: b96fa8410bd53937ddb72a46c02b949691ee9458

krshrimali added 17 commits January 29, 2020 09:52

torch.prod works fine now with fp16 input and fp32 output tensor

dc0e6f3

Replicates sum_kernel_cuda and sum_kernel_impl, adds out_t arg

9141aba

Test added for test_prod_gpu, under review, passes with changes succe…

e770241

…ssfully

modified test for torch.prod (only CUDA), uses all dtype combinations

aa9740d

use torch.tensor.dtype instead of torch.tensor.type()

f7f4876

torch.prod works fine now with fp16 input and fp32 output tensor

99fd726

Replicates sum_kernel_cuda and sum_kernel_impl, adds out_t arg

7c6607d

Test added for test_prod_gpu, under review, passes with changes succe…

f46c73a

…ssfully

modified test for torch.prod (only CUDA), uses all dtype combinations

d5e11eb

use torch.tensor.dtype instead of torch.tensor.type()

f26719c

Merge branch 'master' of https://github.com/krshrimali/pytorch

15d5c1a

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

dcf6acf

Merge remote-tracking branch 'upstream/master'

5fc012a

adding test_torch back to master

58f7b06

Merge remote-tracking branch 'upstream/master'

87dd4af

OpInfo for diag_embed

48b1056

diag_embed OpInfo

b9cf5a6

krshrimali requested a review from kshitij12345 May 20, 2021 04:54

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

krshrimali commented May 20, 2021

View reviewed changes

pytorchbot added the open source label May 20, 2021

kshitij12345 reviewed May 20, 2021

View reviewed changes

sopports_forward_ad True for diag_embed

354d5ed

OpInfo for diag

e41fa6d

krshrimali changed the title ~~OpInfo: diag_embed~~ OpInfo: diag_embed, diagonal May 20, 2021

krshrimali changed the title ~~OpInfo: diag_embed, diagonal~~ [WIP] OpInfo: diag_embed, diagonal May 20, 2021

OpInfo for diagonal, merging conflicts

5ac3ed4

Sample inputs merged for diagonal and diag_embed

1386ea2

krshrimali changed the title ~~[WIP] OpInfo: diag_embed, diagonal~~ OpInfo: diag_embed, diagonal May 20, 2021

krshrimali requested a review from kshitij12345 May 20, 2021 11:05

kshitij12345 reviewed May 20, 2021

View reviewed changes

Address review, remove low and high, tested OK

bd4b4e4

kshitij12345 approved these changes May 20, 2021

View reviewed changes

krshrimali added 2 commits May 20, 2021 18:13

Minor nits, address review

8aa6046

Import chain from itertools

87cc490

krshrimali requested a review from mruberry May 20, 2021 18:08

mruberry approved these changes May 23, 2021

View reviewed changes

Merge conflicts resolved

a65db01

facebook-github-bot closed this in b9d1ad9 May 25, 2021

facebook-github-bot added the Merged label May 25, 2021

github-actions bot deleted the diag-embed branch February 11, 2024 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpInfo: `diag_embed`, `diagonal` #58642

OpInfo: `diag_embed`, `diagonal` #58642

krshrimali commented May 20, 2021

facebook-github-bot commented May 20, 2021 •

edited

Loading

krshrimali May 20, 2021

kshitij12345 left a comment •

edited

Loading

mruberry commented May 20, 2021

krshrimali commented May 20, 2021

krshrimali commented May 20, 2021 •

edited

Loading

krshrimali commented May 20, 2021

kshitij12345 left a comment

kshitij12345 May 20, 2021

krshrimali May 20, 2021

kshitij12345 May 20, 2021

krshrimali May 20, 2021

kshitij12345 May 20, 2021

mruberry May 20, 2021

krshrimali May 20, 2021

kshitij12345 May 20, 2021

mruberry May 20, 2021

kshitij12345 May 20, 2021

kshitij12345 May 20, 2021

krshrimali May 20, 2021

kshitij12345 commented May 20, 2021

kshitij12345 commented May 20, 2021

krshrimali commented May 20, 2021 •

edited

Loading

kshitij12345 left a comment

kshitij12345 May 20, 2021 •

edited

Loading

krshrimali May 20, 2021

kshitij12345 May 20, 2021

krshrimali May 20, 2021

mruberry left a comment

facebook-github-bot commented May 23, 2021

facebook-github-bot commented May 24, 2021

krshrimali commented May 25, 2021

facebook-github-bot commented May 25, 2021

		@@ -2913,6 +2913,25 @@ def sample_inputs_diag(op_info, device, dtype, requires_grad, **kwargs):

		return samples + [vec_sample]

		def sample_inputs_diagonal(op_info, device, dtype, requires_grad, **kwargs):

OpInfo: diag_embed, diagonal #58642

OpInfo: diag_embed, diagonal #58642

Conversation

krshrimali commented May 20, 2021

facebook-github-bot commented May 20, 2021 • edited Loading

💊 CI failures summary and remediations

1 failure not recognized by patterns:

Choose a reason for hiding this comment

kshitij12345 left a comment • edited Loading

Choose a reason for hiding this comment

mruberry commented May 20, 2021

krshrimali commented May 20, 2021

krshrimali commented May 20, 2021 • edited Loading

krshrimali commented May 20, 2021

kshitij12345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshitij12345 commented May 20, 2021

kshitij12345 commented May 20, 2021

krshrimali commented May 20, 2021 • edited Loading

kshitij12345 left a comment

Choose a reason for hiding this comment

kshitij12345 May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 23, 2021

facebook-github-bot commented May 24, 2021

krshrimali commented May 25, 2021

facebook-github-bot commented May 25, 2021

OpInfo: `diag_embed`, `diagonal` #58642

OpInfo: `diag_embed`, `diagonal` #58642

facebook-github-bot commented May 20, 2021 •

edited

Loading

kshitij12345 left a comment •

edited

Loading

krshrimali commented May 20, 2021 •

edited

Loading

krshrimali commented May 20, 2021 •

edited

Loading

kshitij12345 May 20, 2021 •

edited

Loading