mmcv icon indicating copy to clipboard operation
mmcv copied to clipboard

Published 20 hours ago • verified publisher icon open-mmlab

Support receptive field search of CNN models. (TPAMI paper rf-next)

Open gasvn opened this issue 2 years ago • 6 comments

Motivation

Merging a general receptive field search method to mmcv and mmdet. (paper: RF-Next: Efficient Receptive Field Search for Convolutional Neural Networks TPAMI 2022 pdf)

Modification

The RFSearch module is included.

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.

This pr is a part of pr in mmdet https://github.com/open-mmlab/mmdetection/pull/8191

Checklist

Before PR:

  • [x] I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
  • [x] 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, including docstring or example tutorials.

After PR:

  • [x] If the modification has potential influence on downstream or other related projects, this PR should be tested with some of those projects, like MMDet or MMCls.
  • [x] CLA has been signed and all committers have signed the CLA in this PR.

gasvn avatar Jun 15 '22 04:06 gasvn

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 15 '22 04:06 CLAassistant

Hi @gasvn , thanks for your contributions. Please add docstring and type hints for your added files.

zhouzaida avatar Jun 15 '22 12:06 zhouzaida

Hi @gasvn , thanks for your contributions. Please add docstring and type hints for your added files.

I have added docstring and type hints.

gasvn avatar Jun 16 '22 14:06 gasvn

Is there any further update about this pull request?

gasvn avatar Jul 01 '22 03:07 gasvn

Is there anything I can do to make some progress? Thanks~

gasvn avatar Aug 29 '22 10:08 gasvn

Is there anything I can do to make some progress? Thanks~

Hi @gasvn , sorry for my late reply. I'm thinking about where to put the implementation of this hook.

zhouzaida avatar Aug 29 '22 16:08 zhouzaida

Hi, I have solved the comments above. Please let me know if there is anything I can do. Thanks.

gasvn avatar Oct 03 '22 07:10 gasvn

Hi, I have solved the comments above. Please let me know if there is anything I can do. Thanks.

Hi @gasvn , sorry for my late reply.

zhouzaida avatar Oct 26 '22 16:10 zhouzaida

Hi @gasvn , is there any progress?

zhouzaida avatar Nov 17 '22 06:11 zhouzaida

Hi @gasvn , is there any progress? Sorry for the delay. We have fixed the issues you mentioned. And we add the support to asymmetric kernel size (1 x n) to support more networks.

gasvn avatar Nov 17 '22 09:11 gasvn

@zhouzaida It seems the old pre-commit check we used is out of data, so the revised code cannot pass the lint check. Do we need to pull the latest update from mmcv? Or we just keep it as it is?

gasvn avatar Nov 17 '22 09:11 gasvn

@zhouzaida It seems the old pre-commit check we used is out of data, so the revised code cannot pass the lint check. Do we need to pull the latest update from mmcv? Or we just keep it as it is?

It seems the lint passed.

zhouzaida avatar Nov 17 '22 11:11 zhouzaida

@zhouzaida It seems the old pre-commit check we used is out of data, so the revised code cannot pass the lint check. Do we need to pull the latest update from mmcv? Or we just keep it as it is?

It seems the lint passed.

Thanks, we have solved it.

gasvn avatar Nov 17 '22 12:11 gasvn

@zhouzaida This pull request has been processed for several months. Is there a rough schedule for finishing this? Thanks~

gasvn avatar Nov 19 '22 03:11 gasvn

@zhouzaida This pull request has been processed for several months. Is there a rough schedule for finishing this? Thanks~

Hi @gasvn , sorry for my late reply. Thanks for your contribution. This PR will be merged this month.

zhouzaida avatar Nov 19 '22 11:11 zhouzaida

Could we add some unit tests to cover those modifications?

zhouzaida avatar Nov 19 '22 11:11 zhouzaida

Hello, we have fixed the issues following your suggestions. Thanks. @zhouzaida

lzyhha avatar Nov 19 '22 15:11 lzyhha

Could we add some unit tests to cover those modifications?

It is nice to add some unit tests. But I wonder who should do this job, me or you? If we should do this job, is there any guidance to complete this job. @zhouzaida

lzyhha avatar Nov 19 '22 15:11 lzyhha

Could we add some unit tests to cover those modifications?

It is nice to add some unit tests. But I wonder who should do this job, me or you? If we should do this job, is there any guidance to complete this job. @zhouzaida

Hi @lzyhha , it is best for you to add if you have free time. https://github.com/open-mmlab/mmcv/tree/master/tests/test_cnn and https://github.com/open-mmlab/mmcv/blob/master/tests/test_runner/test_hooks.py would be helpful.

zhouzaida avatar Nov 19 '22 17:11 zhouzaida

Ok, I'll add the corresponding unit tests within a few days.

lzyhha avatar Nov 20 '22 01:11 lzyhha

Above issues have been fixed.

lzyhha avatar Nov 20 '22 03:11 lzyhha

Are there any other issues we should do apart from unit tests? @zhouzaida

lzyhha avatar Nov 20 '22 04:11 lzyhha

Hello, we have added unit tests for the rfsearch. Please let me know if there are any issues or anything things I can do. Thanks. @zhouzaida

lzyhha avatar Nov 21 '22 01:11 lzyhha

All above issues are fixed.

lzyhha avatar Nov 21 '22 08:11 lzyhha

Please move the file tests/test_cnn/test_rfsearch_operator.py to tests/test_cnn/test_rfsearch/test_operator.py and those modifications in tests/test_runner/test_hooks.py to tests/test_cnn/test_rfsearch/test_search.py

zhouzaida avatar Nov 21 '22 08:11 zhouzaida

Please move the file tests/test_cnn/test_rfsearch_operator.py to tests/test_cnn/test_rfsearch/test_operator.py and those modifications in tests/test_runner/test_hooks.py to tests/test_cnn/test_rfsearch/test_search.py

I have completed it.

lzyhha avatar Nov 21 '22 08:11 lzyhha

Hi @lzyhha , have you tested this latest commit with https://github.com/open-mmlab/mmdetection/pull/8191?

zhouzaida avatar Nov 21 '22 11:11 zhouzaida

Hi @lzyhha , have you tested this latest commit with open-mmlab/mmdetection#8191?

I will test with official mmdetection and our RF-mmdetection as soon as possible.

RF-mmdetection need some corresponding changes, which will be updated later.

lzyhha avatar Nov 21 '22 11:11 lzyhha

Hello, it seems that there is an error in build_cu102 (Error: retrieving gpg key timed out.). Is it a package that failed to download? @zhouzaida

lzyhha avatar Nov 21 '22 12:11 lzyhha

I have tested this latest commit with RF-mmdetection, and everything is OK. And the RF-mmdetection has been updated following the changes in this latest commit.

And what is the schedule to merge this PR?

@zhouzaida

lzyhha avatar Nov 21 '22 14:11 lzyhha