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

[Enhancement] Add build_func for UPSAMPLE_LAYERS #1272

Closed
wants to merge 6 commits into from

Conversation

Junjun2016
Copy link
Contributor

@Junjun2016 Junjun2016 commented Aug 15, 2021

Motivation

Add build_func for UPSAMPLE_LAYERS

Modification

UPSAMPLE_LAYERS

BC-breaking (Optional)

Does the modification introduce changes that break the backward-compatibility of the downstream repos?
Not sure.

Use cases

  • Before the PR
from mmcv.cnn import build_upsample_layer
build_upsample_layer(cfg, *args, **kwargs)
  • After the PR
from mmcv.cnn.bricks.registry import UPSAMPLE_LAYERS
UPSAMPLE_LAYERS.build(cfg, *args, **kwargs)

@Junjun2016
Copy link
Contributor Author

@xvjiarui

@codecov
Copy link

codecov bot commented Aug 15, 2021

Codecov Report

Merging #1272 (4f4e31a) into master (54907a3) will increase coverage by 0.40%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1272      +/-   ##
==========================================
+ Coverage   68.42%   68.82%   +0.40%     
==========================================
  Files         160      161       +1     
  Lines       10610    10747     +137     
  Branches     1938     1972      +34     
==========================================
+ Hits         7260     7397     +137     
+ Misses       2975     2962      -13     
- Partials      375      388      +13     
Flag Coverage Δ
unittests 68.82% <100.00%> (+0.40%) ⬆️

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

Impacted Files Coverage Δ
mmcv/cnn/bricks/registry.py 100.00% <100.00%> (ø)
mmcv/cnn/bricks/upsample.py 100.00% <100.00%> (ø)
mmcv/utils/config.py 90.10% <0.00%> (-0.29%) ⬇️
mmcv/ops/psa_mask.py 31.81% <0.00%> (ø)
mmcv/runner/builder.py 100.00% <0.00%> (ø)
mmcv/utils/registry.py 98.31% <0.00%> (ø)
mmcv/runner/__init__.py 100.00% <0.00%> (ø)
mmcv/runner/hooks/__init__.py 100.00% <0.00%> (ø)
mmcv/runner/default_constructor.py 77.77% <0.00%> (ø)
mmcv/video/processing.py 77.41% <0.00%> (+0.37%) ⬆️
... and 7 more

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 54907a3...4f4e31a. Read the comment docs.

@xvjiarui xvjiarui self-requested a review August 15, 2021 16:45
@zhouzaida zhouzaida mentioned this pull request Aug 26, 2021
16 tasks
Copy link
Collaborator

@zhouzaida zhouzaida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

'deprecated. Please use '
'``UPSAMPLE_LAYERS.build(cfg, *args, **kwargs)`` instead.'))

return build_upsample_layer_from_cfg(cfg, UPSAMPLE_LAYERS, *args, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use UPSAMPLE_LAYERS.buil() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will lead to recursive import.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then please leave a comment above this line of code.


def build_upsample_layer_from_cfg(cfg, registry, *args, **kwargs):
"""Build upsample layer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add docstring to indicates what is the difference between build_upsample_layer_from_cfg and build_from_cfg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the interfaces of all UPSAMPLE_LAYERs modules are different.
I have added usage and deprecate information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wrap build_upsample_layer as a build_func (build_upsample_layer_from_cfg) and build_upsample_layer can be deprecated in the future.

@ZwwWayne
Copy link
Collaborator

ZwwWayne commented Sep 7, 2021

  1. The motivation of this PR is not clearly delivered through the PR message and the code comments/docstring. Does it want to deprecate build_upsample_layer or what? Please write accurate PR messages and add comments in the code docstring.
  2. not sure is not an acceptable answer for the BC-breaking section. Please be responsible, think about it seriously, and answer it accurately.
  3. The modification section is vague and inconsistent with the real code change. Please rephrase it.
  4. Put build_upsample_layer_from_cfg in registry.py is not a good design to me, please re-think it.
  5. This PR is closed as it dis-obey many rules. Please create a new PR with a better design and more serious/clear PR message. See a good example at [Feature] Add revert_sync_batchnorm #1253

@ZwwWayne ZwwWayne closed this Sep 7, 2021
@zhouzaida zhouzaida mentioned this pull request Sep 13, 2021
16 tasks
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.

4 participants