OpInfo for `nn.functional.avg_pool2d` #62455

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 zou3519 and heitorschueroff 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpInfo for `nn.functional.avg_pool2d` #62455

OpInfo for `nn.functional.avg_pool2d` #62455

krshrimali commented Jul 30, 2021

facebook-github-bot commented Jul 30, 2021 •

edited

Loading

mruberry commented Jul 30, 2021

zou3519 Jul 30, 2021

krshrimali Jul 30, 2021

heitorschueroff Jul 30, 2021

krshrimali Jul 30, 2021

heitorschueroff Jul 30, 2021

zou3519 Jul 30, 2021

krshrimali Jul 30, 2021

krshrimali Jul 30, 2021

heitorschueroff Jul 30, 2021

zou3519 Jul 30, 2021

krshrimali Jul 30, 2021

heitorschueroff left a comment

krshrimali commented Jul 30, 2021

krshrimali commented Aug 4, 2021

facebook-github-bot commented Aug 4, 2021

heitorschueroff commented Aug 4, 2021

facebook-github-bot commented Aug 5, 2021

ngimel commented Aug 6, 2021

OpInfo for nn.functional.avg_pool2d #62455

OpInfo for nn.functional.avg_pool2d #62455

Conversation

krshrimali commented Jul 30, 2021

facebook-github-bot commented Jul 30, 2021 • edited Loading

🔗 Helpful links

💊 CI failures summary and remediations

1 failure not recognized by patterns:

ci.pytorch.org: 1 failed

mruberry commented Jul 30, 2021

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

heitorschueroff left a comment

Choose a reason for hiding this comment

krshrimali commented Jul 30, 2021

krshrimali commented Aug 4, 2021

facebook-github-bot commented Aug 4, 2021

heitorschueroff commented Aug 4, 2021

facebook-github-bot commented Aug 5, 2021

ngimel commented Aug 6, 2021

OpInfo for `nn.functional.avg_pool2d` #62455

OpInfo for `nn.functional.avg_pool2d` #62455

facebook-github-bot commented Jul 30, 2021 •

edited

Loading