OpInfo for `nn.functional.avg_pool2d` by krshrimali · Pull Request #62455 · pytorch/pytorch

krshrimali · 2021-07-30T05:57:56Z

Please see pytorch/functorch#78

cc: @mruberry @zou3519

facebook-github-bot · 2021-07-30T05:58:02Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/62455
📄 Preview docs built from this PR

💊 CI failures summary and remediations

As of commit a2a23d5 (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-py3-clang5-android-ndk-r19c-gradle-custom-build-single-full-jit}	^{pytorch android gradle custom build single architecture (for PR)}	🔁 rerun

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm4.2-py3.6

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.

mruberry · 2021-07-30T06:28:55Z

@zou3519 and @heitorschueroff would you review this?

zou3519 · 2021-07-30T13:46:56Z

torch/testing/_internal/common_methods_invocations.py

+    cases = (((1, 3, 9, 9), (3, 3), (1, 1), (1, ), True, False, 2),
+             ((1, 1, 4, 4), (2, 2), (1, 1), (0, ), False, True, -2),
+             ((1, 2, 6, 6), (4, 4), (2, 2), (2, ), True, True, 3))


Let's add some more cases.

Could we have one that is just a single kernel size ((1, 3, 9, 9),)?

avgpool_2d advertises that stride, padding, and kernel_size can be a single number, so we should test something like ((1, 3, 9, 9), 3, 1, 1)

Could we have a case where the two strides are not the same and another where the two paddings are not the same?

We aren't testing division_override=None

Thanks for taking a look @zou3519! I've addressed the suggestions in the latest commit. :)

heitorschueroff · 2021-07-30T15:37:55Z

torch/testing/_internal/common_methods_invocations.py

+def sample_inputs_avgpool2d(op_info, device, dtype, requires_grad, **kwargs):
+    make_arg = partial(make_tensor, device=device, dtype=dtype, requires_grad=requires_grad)
+
+    cases = (((1, 3, 9, 9), (3, 3), (1, 1), (1, ), True, False, 2),


Can you add a comment describing what each element represents please

Done, thanks for the suggestion @heitorschueroff!

heitorschueroff · 2021-07-30T15:39:10Z

torch/testing/_internal/common_methods_invocations.py

+             ((1, 1, 4, 4), (2, 2), (1, 1), (0, ), False, True, -2),
+             ((1, 2, 6, 6), (4, 4), (2, 2), (2, ), True, True, 3))
+
+    return [SampleInput(make_arg(input_shape), args=(kernel_size, stride, padding, ceil_mode,


nit: can you do this with a regular for loop instead of a list comprehension for readability.

I really like list comprehensions. I think whether or not to do a list comprehension or do a for-loop should not be something we suggest in reviews because it's a matter of personal taste and there is no official PyTorch Style Guide.

Done, thanks for the suggestion, @heitorschueroff!

I'm sorry, I saw your comment just now @zou3519! Since I had to add a single case without any args except kernel size as well, so I kept a generator() approach for now. Hope that's fine.

I really like list comprehensions. I think whether or not to do a list comprehension or do a for-loop should not be something we suggest in reviews because it's a matter of personal taste and there is no official PyTorch Style Guide.

List comprehensions are great for short one-liners, but I also agree that it is a matter of preferred style and since we don't enforce a style guide it is ok to ignore this suggestion.

Either style works, so this is fine :). I wouldn't say that one is nicer than the other because it depends on who is reading the code

Thanks for the valuable inputs, @zou3519 and @heitorschueroff! I appreciate it. :)

heitorschueroff

@krshrimali This is looking great, thank you for the contribution! I think there is room to explore a little more with the test cases, for instance, it looks like all test cases have size 1 for batch dimension. However, as a first PR adding OpInfo this is already good. Let's wait for the test to run. Thanks!

krshrimali · 2021-07-30T17:29:45Z

@krshrimali This is looking great, thank you for the contribution! I think there is room to explore a little more with the test cases, for instance, it looks like all test cases have size 1 for batch dimension. However, as a first PR adding OpInfo this is already good. Let's wait for the test to run. Thanks!

Thanks, @heitorschueroff! I had two more cases in mind which I added in the recent commit:

Since stride has a default value of kernel_size, passing an empty tuple (for stride) has been added in the cases. None can not be passed as JIT test will fail ( Expected a value of type 'List[int]' for argument 'stride' but instead found type 'NoneType'.).
Multiple batch size, as also suggested by you. I think it's good for completion. (used 2 as the batch size, anything greater would be just extra load on the testing time, I thought).

cc: @zou3519 as well.

Hope this looks complete now. Thank you!

krshrimali · 2021-08-04T03:25:43Z

Gentle ping: @zou3519 and @heitorschueroff. The failures seem irrelevant to this PR, please let me know if there is anything else required here.

Thanks for your reviews and help.

facebook-github-bot · 2021-08-04T07:59:57Z

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

heitorschueroff · 2021-08-04T08:00:25Z

Gentle ping: @zou3519 and @heitorschueroff. The failures seem irrelevant to this PR, please let me know if there is anything else required here.

Thanks for your reviews and help.

LGTM! We appreciate your contribution.

facebook-github-bot · 2021-08-05T17:30:29Z

@heitorschueroff merged this pull request in 5dbcd56.

ngimel · 2021-08-06T22:10:06Z

Reverting, causes flakiness (adaptive_avg_pool backward is nondeterministic)

Avg pool2d init

3e392cc

facebook-github-bot added the cla signed label Jul 30, 2021

krshrimali marked this pull request as draft July 30, 2021 05:58

pytorchbot added the open source label Jul 30, 2021

More inputs and args coverage

987aa03

mruberry requested review from heitorschueroff and zou3519 July 30, 2021 06:28

zou3519 reviewed Jul 30, 2021

View reviewed changes

heitorschueroff reviewed Jul 30, 2021

View reviewed changes

Address reviews: comments, more edge cases covered

94ccf5d

krshrimali marked this pull request as ready for review July 30, 2021 17:06

heitorschueroff approved these changes Jul 30, 2021

View reviewed changes

use default for stride in one case, and 2 batch size for another

a2a23d5

krshrimali added the module: testing Issues related to the torch.testing module (not tests) label Jul 30, 2021

facebook-github-bot closed this in 5dbcd56 Aug 5, 2021

facebook-github-bot added the Merged label Aug 5, 2021

zou3519 mentioned this pull request Aug 5, 2021

Top 25 OpInfos for functorch pytorch/functorch#78

Closed

23 tasks

Conversation

krshrimali commented Jul 30, 2021

Uh oh!

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

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

1 failure not recognized by patterns:

ci.pytorch.org: 1 failed

Uh oh!

mruberry commented Jul 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heitorschueroff left a comment

Choose a reason for hiding this comment

Uh oh!

krshrimali commented Jul 30, 2021

Uh oh!

krshrimali commented Aug 4, 2021

Uh oh!

facebook-github-bot commented Aug 4, 2021

Uh oh!

heitorschueroff commented Aug 4, 2021

Uh oh!

facebook-github-bot commented Aug 5, 2021

Uh oh!

ngimel commented Aug 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

facebook-github-bot commented Jul 30, 2021 •

edited

Loading