Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameters of DilatedEncoder #7812

Merged
merged 5 commits into from
May 11, 2022
Merged

Parameters of DilatedEncoder #7812

merged 5 commits into from
May 11, 2022

Conversation

Shanyaliux
Copy link
Contributor

@Shanyaliux Shanyaliux commented Apr 24, 2022

Add the original fixed parameter block_dilations as an incoming parameter

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

When I used yolof, I modified the modification and found that the parameter named block_dilations of DilatedEncoder is fixed. If I have num_residual_blocks as other values, this parameter cannot be very convenient.

Modification

I modified this parameter of DilatedEncoder to be an incoming parameter and added a default value in its config file.

BC-breaking (Optional)

Does the modification introduce changes that break the backward-compatibility of the downstream repos?
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

  1. Pre-commit or other linting tools are used to fix the potential lint issues.
  2. The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  3. If the modification has potential influence on downstream projects, this PR should be tested with downstream projects, like MMDet or MMCls.
  4. The documentation has been modified accordingly, like docstring or example tutorials.

Add the original fixed parameter block_dilations as an incoming parameter
@CLAassistant
Copy link

CLAassistant commented Apr 24, 2022

CLA assistant check
All committers have signed the CLA.

@Shanyaliux
Copy link
Contributor Author

In the test process after the first submission, I noticed that the error was reported because the new block_dilations was missing when its test function called DilatedEncoder.
The error message is as follows:

    def test_dilated_encoder():
        in_channels = 16
        out_channels = 32
        out_shape = 34
>       dilated_encoder = DilatedEncoder(in_channels, out_channels, 16, 2)
E       TypeError: __init__() missing 1 required positional argument: 'block_dilations'

So I fixed the bug in the latest commit

@Shanyaliux
Copy link
Contributor Author

I noticed that you released v2.24.0, do I need to resubmit on the new version?

@ZwwWayne ZwwWayne changed the base branch from master to dev May 8, 2022 05:42
[fix] tests/test_models/test_necks.py:237:80: E501 line too long (84 > 79 characters)
@codecov
Copy link

codecov bot commented May 8, 2022

Codecov Report

Merging #7812 (e17bae3) into dev (73b4e65) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head e17bae3 differs from pull request most recent head 942701e. Consider uploading reports for the commit 942701e to get more accurate results

@@            Coverage Diff             @@
##              dev    #7812      +/-   ##
==========================================
- Coverage   64.71%   64.67%   -0.04%     
==========================================
  Files         351      351              
  Lines       28461    28461              
  Branches     4807     4807              
==========================================
- Hits        18418    18408      -10     
- Misses       9055     9066      +11     
+ Partials      988      987       -1     
Flag Coverage Δ
unittests 64.67% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmdet/models/necks/dilated_encoder.py 82.35% <100.00%> (ø)
mmdet/core/bbox/samplers/random_sampler.py 75.00% <0.00%> (-5.56%) ⬇️
mmdet/models/detectors/cornernet.py 94.87% <0.00%> (-5.13%) ⬇️
mmdet/core/bbox/samplers/sampling_result.py 73.01% <0.00%> (-1.59%) ⬇️
mmdet/models/dense_heads/corner_head.py 68.33% <0.00%> (-1.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73b4e65...942701e. Read the comment docs.

@Shanyaliux
Copy link
Contributor Author

Sorry, I don't know how to solve this error, it shouldn't be my problem.
1
2

@ZwwWayne
Copy link
Collaborator

Sorry, I don't know how to solve this error, it shouldn't be my problem. 1 2

Do not worry, it is caused by other issues and have been fixed in dev branch.

1 similar comment
@ZwwWayne
Copy link
Collaborator

Sorry, I don't know how to solve this error, it shouldn't be my problem. 1 2

Do not worry, it is caused by other issues and have been fixed in dev branch.

@ZwwWayne ZwwWayne merged commit d7734dd into open-mmlab:dev May 11, 2022
ZwwWayne pushed a commit that referenced this pull request Jul 18, 2022
* Parameters of DilatedEncoder

Add the original fixed parameter block_dilations as an incoming parameter

* [Fix] Add default value to block_dilations parameter of DilatedEncoder and fully function annotation

* [Fix] The incoming parameter of calling DilatedEncoder in the test_dilated_encoder function is missing block_dilations

* Update test_necks.py

[fix] tests/test_models/test_necks.py:237:80: E501 line too long (84 > 79 characters)
ZwwWayne pushed a commit to ZwwWayne/mmdetection that referenced this pull request Jul 19, 2022
* Parameters of DilatedEncoder

Add the original fixed parameter block_dilations as an incoming parameter

* [Fix] Add default value to block_dilations parameter of DilatedEncoder and fully function annotation

* [Fix] The incoming parameter of calling DilatedEncoder in the test_dilated_encoder function is missing block_dilations

* Update test_necks.py

[fix] tests/test_models/test_necks.py:237:80: E501 line too long (84 > 79 characters)
ZwwWayne pushed a commit to ZwwWayne/mmdetection that referenced this pull request Jul 19, 2022
* Parameters of DilatedEncoder

Add the original fixed parameter block_dilations as an incoming parameter

* [Fix] Add default value to block_dilations parameter of DilatedEncoder and fully function annotation

* [Fix] The incoming parameter of calling DilatedEncoder in the test_dilated_encoder function is missing block_dilations

* Update test_necks.py

[fix] tests/test_models/test_necks.py:237:80: E501 line too long (84 > 79 characters)
SakiRinn pushed a commit to SakiRinn/mmdetection-locount that referenced this pull request Mar 17, 2023
* Parameters of DilatedEncoder

Add the original fixed parameter block_dilations as an incoming parameter

* [Fix] Add default value to block_dilations parameter of DilatedEncoder and fully function annotation

* [Fix] The incoming parameter of calling DilatedEncoder in the test_dilated_encoder function is missing block_dilations

* Update test_necks.py

[fix] tests/test_models/test_necks.py:237:80: E501 line too long (84 > 79 characters)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants