[Enhancement] Lower the restrictions of _resize method in BaseMergeCell #1959

JarvisKevin · 2022-05-14T11:31:33Z

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

Fix the bug met in using nasfpn which is mentioned at open-mmlab/mmdetection#3401.

Modification

Avoid the strong restrictions of _resize function in BaseMergeCell:

When Downsampling the feature map, the feature map's shape must be divisible by the target size. We pad zero around feature map before max_pool2d opt to make it always divisible. (line 102 ~ 107)
Considering the different downsampling scale of H and W, shape[-2] and shape[-1] are involed in the definition of kernel_size. (line 110)

BC-breaking (Optional)

Does the modification introduce changes that break the backward-compatibility of the downstream repositories?
If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.

Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.

Checklist

Before PR:

I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
Pre-commit or linting tools indicated in CONTRIBUTING.md are used to fix the potential lint issues.
Bug fixes are covered by unit tests, the case that causes the bug should be added in the unit tests.
New functionalities are covered by complete unit tests. If not, please add more unit test to ensure the correctness.
The documentation has been modified accordingly, inclu

Fix the bug met in using nasfpn which is mentioned at open-mmlab/mmdetection#5987. Avoid the strong restrictions of _resize function in BaseMergeCell: 1. When Downsampling the feature map, the feature map's shape must be divisible by the target size. We pad zero around feature map before max_pool2d opt to make it always divisible. (line 102 ~ 107) 2. Considering the different downsampling scale of H and W, shape[-2] and shape[-1] are involed in the definition of kernel_size. (line 110)

CLAassistant · 2022-05-14T11:31:37Z

All committers have signed the CLA.

check flake8 & isort

yapf

mmcv/ops/merge_cells.py

HAOCHENYE · 2022-05-14T13:51:32Z

mmcv/ops/merge_cells.py

+                x = nn.ConstantPad2d(
+                    (padding_left, padding_right, padding_top, padding_bottom),
+                    0)(
+                        x)


Could we use F.pad instead?

Thanks for your suggestion, I will use the func from F replacing the one from nn.

HAOCHENYE

Thanks for your contribution! It could be better to add a unit test in tests/test_ops/test_merge_cell.py to test the case that x1 or x2 cannot be devised by size

X_pad rename to padding_x Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com>

format the code style after renameing the X_pad to padding_x

Mainly test the downsampling resize in BaseMergeCell. The smaller target size is set to (14, 7), the classical feature map's size in the last few stages of the backbone, which will product different downsampling scales in different dims.

JarvisKevin · 2022-05-16T01:47:38Z

@HAOCHENYE Why the unit_test, run lasted 22h, hasn't finished? Is there anything wrong with my code?

HAOCHENYE · 2022-05-16T09:51:42Z

mmcv/ops/merge_cells.py

+            if x.shape[-2] % size[-2] != 0 or x.shape[-1] % size[-1] != 0:
+                h, w = x.shape[-2:]
+                h_t, w_t = size
+                padding_h = (h // h_t + 1) * h_t - h
+                padding_w = (w // w_t + 1) * w_t - w
+                padding_left = padding_w // 2
+                padding_right = padding_w - padding_left
+                padding_top = padding_h // 2
+                padding_bottom = padding_h - padding_top
+                pad = (padding_left, padding_right, padding_top,
+                       padding_bottom)
+                x = F.pad(x, pad, mode='constant', value=0.0)
+            kernel_size = (x.shape[-2] // size[-2], x.shape[-1] // size[-1])


Suggest to keep naming rule consistent. Here we define padding_right with full name "right" but padding_h with Abbreviations h。It could be better to rename padding_h to padding_heigth and padding_w to padding_width. Besides, does h_t mean target height? If so, target_height could be better.

HAOCHENYE · 2022-05-16T09:59:17Z

tests/test_ops/test_merge_cells.py

-    inputs_x = torch.randn([2, 256, 32, 32])
+    inputs_x = torch.randn([2, 256, 14, 7])


It could be better to test SumCell and ConcatCell with divisible and indivisible both

HAOCHENYE · 2022-05-16T10:03:22Z

tests/test_ops/test_merge_cells.py

+    """resize to a smaller size by which the inputs_x can't be divisible.
+    And There're different downsample scale on dim H & dim W.
+    """


This should be comment like # resize to a smaller... but not docstring starts with """

HAOCHENYE · 2022-05-16T10:04:02Z

tests/test_ops/test_merge_cells.py

+    """resize to a smaller size by which the inputs_x can't be divisible.
+    And There're different downsample scale on dim H & dim W.
+    """
+    target_size = (14, 7)


We should test divisible and indivisible size both.

format the variable name

Testing divisible and indivisible situations simultaneously

jshilong

LGTM

mmcv/ops/merge_cells.py

fix the bug when h is indivisible and w is divisible, the pad_w will be padded unreasonable. Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>

fix the bug when w is indivisible and h is divisible, the pad_h will be padded unreasonable. Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>

mmcv/ops/merge_cells.py

zhouzaida · 2022-05-20T02:48:28Z

Hi, lint failed. After fixing the lint and refactoring the unit tests with pytest.mark.parametrize to avoid using for clause, this PR will be merged.

make pad_h, pad_w more readable

use @pytest.mark.parametrize instead of 'for' methor

isort

…ll (open-mmlab#1959) * Fix the bug met in using nasfpn Fix the bug met in using nasfpn which is mentioned at open-mmlab/mmdetection#5987. Avoid the strong restrictions of _resize function in BaseMergeCell: 1. When Downsampling the feature map, the feature map's shape must be divisible by the target size. We pad zero around feature map before max_pool2d opt to make it always divisible. (line 102 ~ 107) 2. Considering the different downsampling scale of H and W, shape[-2] and shape[-1] are involed in the definition of kernel_size. (line 110) * Update merge_cells.py check flake8 & isort * Update merge_cells.py * Update merge_cells.py yapf * Update mmcv/ops/merge_cells.py X_pad rename to padding_x Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com> * Update merge_cells.py format the code style after renameing the X_pad to padding_x * Update test_merge_cells.py Mainly test the downsampling resize in BaseMergeCell. The smaller target size is set to (14, 7), the classical feature map's size in the last few stages of the backbone, which will product different downsampling scales in different dims. * Update test_merge_cells.py add "# Copyright (c) OpenMMLab. All rights reserved." * Update merge_cells.py format the variable name * Update test_merge_cells.py Testing divisible and indivisible situations simultaneously * Update mmcv/ops/merge_cells.py fix the bug when h is indivisible and w is divisible, the pad_w will be padded unreasonable. Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com> * Update mmcv/ops/merge_cells.py fix the bug when w is indivisible and h is divisible, the pad_h will be padded unreasonable. Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com> * fix undefined error * Update merge_cells.py make pad_h, pad_w more readable * Update test_merge_cells.py use @pytest.mark.parametrize instead of 'for' methor * Update merge_cells.py * Update test_merge_cells.py isort * Update merge_cells.py isort Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com> Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>

mm-assistant bot assigned HAOCHENYE May 14, 2022

JarvisKevin added 3 commits May 14, 2022 19:52

Update merge_cells.py

8d913ef

check flake8 & isort

Update merge_cells.py

e5ab75b

Update merge_cells.py

a9067bd

yapf

HAOCHENYE reviewed May 14, 2022

View reviewed changes

mmcv/ops/merge_cells.py Outdated Show resolved Hide resolved

HAOCHENYE reviewed May 14, 2022

View reviewed changes

JarvisKevin and others added 4 commits May 15, 2022 09:08

Update mmcv/ops/merge_cells.py

c8371c9

X_pad rename to padding_x Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com>

Update merge_cells.py

4fb9271

format the code style after renameing the X_pad to padding_x

Update test_merge_cells.py

e05e793

Mainly test the downsampling resize in BaseMergeCell. The smaller target size is set to (14, 7), the classical feature map's size in the last few stages of the backbone, which will product different downsampling scales in different dims.

zhouzaida requested a review from jshilong May 15, 2022 16:01

HAOCHENYE reviewed May 16, 2022

View reviewed changes

JarvisKevin added 2 commits May 16, 2022 20:12

Update merge_cells.py

95f58de

format the variable name

Update test_merge_cells.py

d6959e8

Testing divisible and indivisible situations simultaneously

jshilong approved these changes May 18, 2022

View reviewed changes

HAOCHENYE approved these changes May 18, 2022

View reviewed changes

zhouzaida reviewed May 19, 2022

View reviewed changes

mmcv/ops/merge_cells.py Outdated Show resolved Hide resolved

zhouzaida reviewed May 19, 2022

View reviewed changes

mmcv/ops/merge_cells.py Outdated Show resolved Hide resolved

zhouzaida reviewed May 19, 2022

View reviewed changes

mmcv/ops/merge_cells.py Outdated Show resolved Hide resolved

JarvisKevin and others added 2 commits May 20, 2022 10:07

Update mmcv/ops/merge_cells.py

aad05fe

fix the bug when h is indivisible and w is divisible, the pad_w will be padded unreasonable. Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>

Update mmcv/ops/merge_cells.py

e1931c8

fix the bug when w is indivisible and h is divisible, the pad_h will be padded unreasonable. Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>

zhouzaida reviewed May 20, 2022

View reviewed changes

mmcv/ops/merge_cells.py Outdated Show resolved Hide resolved

fix undefined error

d1a4f48

JarvisKevin added 5 commits May 20, 2022 13:05

Update merge_cells.py

2777af0

make pad_h, pad_w more readable

Update test_merge_cells.py

d806875

use @pytest.mark.parametrize instead of 'for' methor

Update merge_cells.py

ffa2754

Update test_merge_cells.py

5d608c3

isort

Update merge_cells.py

bb50612

isort

zhouzaida approved these changes May 20, 2022

View reviewed changes

HAOCHENYE approved these changes May 20, 2022

View reviewed changes

zhouzaida changed the title ~~Fix the bug met in using nasfpn~~ [Enhancement] Lower the restrictions of _resize method in BaseMergeCell May 20, 2022

zhouzaida merged commit 60eadb0 into open-mmlab:master May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Lower the restrictions of _resize method in BaseMergeCell #1959

[Enhancement] Lower the restrictions of _resize method in BaseMergeCell #1959

JarvisKevin commented May 14, 2022 •

edited by zhouzaida

Loading

CLAassistant commented May 14, 2022 •

edited

Loading

HAOCHENYE May 14, 2022

JarvisKevin May 15, 2022

HAOCHENYE left a comment

JarvisKevin commented May 16, 2022

HAOCHENYE May 16, 2022

HAOCHENYE May 16, 2022 •

edited

Loading

HAOCHENYE May 16, 2022

HAOCHENYE May 16, 2022

jshilong left a comment

zhouzaida commented May 20, 2022

		inputs_x = torch.randn([2, 256, 32, 32])
		inputs_x = torch.randn([2, 256, 14, 7])

[Enhancement] Lower the restrictions of _resize method in BaseMergeCell #1959

[Enhancement] Lower the restrictions of _resize method in BaseMergeCell #1959

Conversation

JarvisKevin commented May 14, 2022 • edited by zhouzaida Loading

Motivation

Modification

BC-breaking (Optional)

Use cases (Optional)

Checklist

CLAassistant commented May 14, 2022 • edited Loading

HAOCHENYE May 14, 2022

Choose a reason for hiding this comment

JarvisKevin May 15, 2022

Choose a reason for hiding this comment

HAOCHENYE left a comment

Choose a reason for hiding this comment

JarvisKevin commented May 16, 2022

HAOCHENYE May 16, 2022

Choose a reason for hiding this comment

HAOCHENYE May 16, 2022 • edited Loading

Choose a reason for hiding this comment

HAOCHENYE May 16, 2022

Choose a reason for hiding this comment

HAOCHENYE May 16, 2022

Choose a reason for hiding this comment

jshilong left a comment

Choose a reason for hiding this comment

zhouzaida commented May 20, 2022

JarvisKevin commented May 14, 2022 •

edited by zhouzaida

Loading

CLAassistant commented May 14, 2022 •

edited

Loading

HAOCHENYE May 16, 2022 •

edited

Loading