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

[Feature] Support various scales in RRDBNet #699

Merged
merged 9 commits into from
Jan 20, 2022

Conversation

anse3832
Copy link
Contributor

@anse3832 anse3832 commented Jan 7, 2022

Motivation

  1. Current RRDBNet only supports 4x upsampling.
  2. The parameter "radius" in config file of real_esrgan is wrong.

Modification

  • Add scale parameter in rrdb_net
  • Edit config file of real_esrgan for supporting various scales
  • Edit config file of real_esrgan for fixing some errors (radius=50 -> kernel_size=51)

Copy link
Member

@ckkelvinchan ckkelvinchan left a comment

Choose a reason for hiding this comment

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

@anse3832 Thank you very much! Would you mind changing also realesrnet_c64b23g32_12x4_lr2e-4_1000k_df2k_ost.py and adding the unittest for the added support here?

Thanks again!

mmedit/models/backbones/sr_backbones/rrdb_net.py Outdated Show resolved Hide resolved
mmedit/models/backbones/sr_backbones/rrdb_net.py Outdated Show resolved Hide resolved
mmedit/models/backbones/sr_backbones/rrdb_net.py Outdated Show resolved Hide resolved
@ckkelvinchan ckkelvinchan changed the title Support various scales in rrdbnet [Feature] Support various scales in RRDBNet Jan 7, 2022
@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #699 (e461ca0) into master (4f814a8) will increase coverage by 1.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
+ Coverage   80.59%   81.81%   +1.22%     
==========================================
  Files         200      211      +11     
  Lines       11419    11896     +477     
  Branches     1831     1912      +81     
==========================================
+ Hits         9203     9733     +530     
+ Misses       1932     1868      -64     
- Partials      284      295      +11     
Flag Coverage Δ
unittests 81.79% <100.00%> (+1.21%) ⬆️

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

Impacted Files Coverage Δ
mmedit/models/backbones/sr_backbones/rrdb_net.py 95.83% <100.00%> (+0.52%) ⬆️
mmedit/datasets/pipelines/crop.py 95.72% <0.00%> (-1.63%) ⬇️
mmedit/datasets/pipelines/blur_kernels.py 83.08% <0.00%> (-1.48%) ⬇️
mmedit/models/__init__.py 100.00% <0.00%> (ø)
mmedit/datasets/__init__.py 100.00% <0.00%> (ø)
mmedit/models/backbones/__init__.py 100.00% <0.00%> (ø)
mmedit/datasets/pipelines/loading.py 96.01% <0.00%> (ø)
mmedit/datasets/pipelines/__init__.py 100.00% <0.00%> (ø)
mmedit/datasets/sr_reds_multiple_gt_dataset.py 100.00% <0.00%> (ø)
...edit/models/backbones/encoder_decoders/__init__.py 100.00% <0.00%> (ø)
... and 17 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 4f814a8...e461ca0. Read the comment docs.

Comment on lines 168 to 192
# x2 model, initialization and forward (cpu)
net = RRDBNet(
in_channels=3,
out_channels=3,
scale=2,
mid_channels=8,
num_blocks=2,
growth_channels=4)
net_init_weights(pretrained=None)
input_shape = (1, 3, 12, 12)
img = _demo_inputs(input_shape)
output = net(img)
assert output.shape == (1, 3, 24, 24)

# x2 model forward (gpu)
if torch.cuda.is_avaliable():
net = net.cuda()
output = net(img.cuda())
assert output.shape == (1, 3, 24, 24)

Copy link
Member

Choose a reason for hiding this comment

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

The unittest does not pass. Here you can simply copy the original one and set scale to 2.

@ckkelvinchan
Copy link
Member

Hi @anse3832, it seems that linting failed. You can first install pre-commit by pip install pre-commit. Then you can type pre-commit install in the root directory of mmediting.

After that every time you commit something, it will check the format for you. Thank you very much for your contributions and we are almost there!

@ckkelvinchan
Copy link
Member

In addition, unittest also failed. You may try to run the test locally to test whether it has any errors~

@nijkah
Copy link
Contributor

nijkah commented Jan 17, 2022

I'm a member of the team with anse3832.
I helped his PR, lint and pytest.
Can you check it?

@ckkelvinchan
Copy link
Member

I'm a member of the team with anse3832. I helped his PR, lint and pytest. Can you check it?

Hi @nijkah, thank you very much! I will check it.

@ckkelvinchan
Copy link
Member

Hi @anse3832 @nijkah, I think it is almost okay. Could you help adding one more unittest, where scale=3? We want to cover the case when scale is not 1,2 or 4

@anse3832
Copy link
Contributor Author

anse3832 commented Jan 18, 2022

@ckkelvinchan
I have some concerns about ESRGAN and Real-ESRGAN

  1. The Real-ESRGAN official code (https://github.com/xinntao/Real-ESRGAN) only works when scale is 1,2 or 4. Therefore, I'm worried about supporting x3 scale.

  2. The ESRGAN and Real-ESRGAN use the same "RRDBNet", but it works slightly different.

  • Real-ESRGAN uses pixelunshuffle, while ESRGAN doesn't
  • Real-ESRGAN always uses nearest upsample twice regardless of scale (1, 2, 4), while ESRGAN use different times of nearest upsample according to scale (once in x2, x3 scale and twice in x4 scale)

Considering above concerns, I suggest newly writing the code for "RRDBNet" in Real-ESRGAN as "RealRRDBNet"

  • When user wants to use ESRGAN, the code calls "RRDBNet" (it supports x2, x3 and x4 scale)
  • When user wants to use Real-ESRGAN, the code calls "RealRRDBNet" (it supports x1, x2 and x4 scale)

Is it okay to proceed these works? If you have any suggestions, please let me know.

@ckkelvinchan
Copy link
Member

Thank you for your suggestions! What I meant in the previous reply is that we want to cover the following line of code.

image

That means we are not going to support x3. We just need to make sure in the unittest that an error will be raised. I checked the official code and it seems that x3 is not supported neither.

@anse3832
Copy link
Contributor Author

@ckkelvinchan
Do you mean that the unittest code should be like below? (it will raise error because of x3)
error

@ckkelvinchan
Copy link
Member

@ckkelvinchan Do you mean that the unittest code should be like below? (it will raise error because of x3) error

When you expect an error, you should use with pytest.raises(). You can follow the code here.

By the way, you do not need to separate RealRRDBNet from RRDBNet. That means, we support only 1x, 2x, and 4x only, and for 1x and 2x, we use an additional pixel-unshuffle.

Thanks again!

@anse3832
Copy link
Contributor Author

anse3832 commented Jan 18, 2022

@ckkelvinchan
In the official ESRGAN code (https://github.com/xinntao/ESRGAN/blob/master/RRDBNet_arch.py), it only supports x4 scale and it does not use pixelunshuffle.

If it is allowed to use pixelunshuffle in ESRGAN (although the official code does not support), I did not need to separate the code for ESRGAN and Real-ESRGAN. In this case, the code supports for x1, x2 or x4 scale. (Neither of official ESRGAN or Real-ESRGAN supports x3 scale)

  • RRDBNet (both in ESRGAN and Real-SRGAN): supports x1, x2 or x4 scale and uses pixelunshuffle

If I should follow the official code, I think it's better to separate the code.

  • RRDBNet (in ESRGAN): supports x4 scale and doesn't use pixelunshuffle
  • RealRRDBNet (in Real-ESRGAN): supports x1, x2 or x4 scale and uses pixelunshuffle

@ckkelvinchan
Copy link
Member

Yes it is allowed to use pixel-unshuffle in ESRGAN. Since this modification does not affect the original ESRGAN. We can extend it to support x1 and x2.

ckkelvinchan
ckkelvinchan previously approved these changes Jan 18, 2022
Copy link
Member

@ckkelvinchan ckkelvinchan left a comment

Choose a reason for hiding this comment

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

Thank you @anse3832 and @nijkah !

@ckkelvinchan
Copy link
Member

Hi @anse3832 it seems that CI failed. Could you help have a look? Thanks again for your help.

@nijkah
Copy link
Contributor

nijkah commented Jan 19, 2022

@ckkelvinchan Fixed. I think it works this time. 🙏

@nijkah
Copy link
Contributor

nijkah commented Jan 19, 2022

Checked lint failure.
@anse3832 Can you approve the PR?
anse3832#4

Copy link
Member

@ckkelvinchan ckkelvinchan left a comment

Choose a reason for hiding this comment

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

Thanks @anse3832 and @nijkah ! All tests passed.

@ckkelvinchan
Copy link
Member

Your efforts are very much appreciated. Look forward to working with you next time!

@ckkelvinchan ckkelvinchan merged commit 8043791 into open-mmlab:master Jan 20, 2022
@wangruohui
Copy link
Member

Hi all, it seems that this PR accidentally changes file permissions of some config files from 644 to 755, can anybody help to fix that?

@nijkah
Copy link
Contributor

nijkah commented Jan 26, 2022

@wangruohui
nijkah@0d41d2c
Is it okay to pull request this branch?

@ckkelvinchan
Copy link
Member

@wangruohui
nijkah@0d41d2c
Is it okay to pull request this branch?

It is fixed in #718 by @wangruohui. Thanks anyways!

Yshuo-Li pushed a commit to Yshuo-Li/mmediting that referenced this pull request Jul 15, 2022
Co-authored-by: nijkah <nijkah@gmail.com>
Co-authored-by: hakjinlee <hakjinlee@si-analytics.ai>
Yshuo-Li pushed a commit to Yshuo-Li/mmediting that referenced this pull request Jul 15, 2022
Co-authored-by: nijkah <nijkah@gmail.com>
Co-authored-by: hakjinlee <hakjinlee@si-analytics.ai>
@OpenMMLab-Assistant003
Copy link

Hi @anse3832!First of all, we want to express our gratitude for your significant PR in the MMEditing project. Your contribution is highly appreciated, and we are grateful for your efforts in helping improve this open-source project during your personal time. We believe that many developers will benefit from your PR.

We would also like to invite you to join our Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. Look forward to seeing you there! Join us :https://discord.gg/UjgXkPWNqA

If you have WeChat account,welcome to join our community on WeChat. You can add our assistant :openmmlabwx. Please add "mmsig + Github ID" as a remark when adding friends:)
Thank you again for your contribution❤ @anse3832

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

5 participants