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

Support Y-channel PSNR and SSIM #250

Merged
merged 3 commits into from
Apr 12, 2021
Merged

Conversation

ckkelvinchan
Copy link
Member

Many existing SR methods compute PSNR and SSIM on Y-channel. However, MMEditing currently does not support this operation. This PR adds this operation by adding an argument color_space in psnr and ssim.

@@ -165,7 +165,7 @@ def reorder_image(img, input_order='HWC'):
return img


def psnr(img1, img2, crop_border=0, input_order='HWC'):
def psnr(img1, img2, crop_border=0, input_order='HWC', color_space=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

The name "color_space" needs a better design.
"Y" is not a color (standard) space

Copy link
Member Author

Choose a reason for hiding this comment

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

How about convert_to? This is what niqe used.

Copy link
Contributor

Choose a reason for hiding this comment

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

convert_to is not good either, think harder

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, convert_to is ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we either keep the input as it is or turn it to y-channel. How about we use a boolean variable named to_y_channel? Not sure if there is a need to support other color spaces in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this should be extensible to other channels. Sometimes we want to compute on Hue or Lightness.
Either we do not support color conversion and extraction in this function and users need to convert them before call this function, or we leave a generic interface (no need to actually implement) so that any possible conversion is possible to implement.

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #250 (fbaaef0) into master (9243ffc) will increase coverage by 0.05%.
The diff coverage is 83.40%.

❗ Current head fbaaef0 differs from pull request most recent head 1dd1009. Consider uploading reports for the commit 1dd1009 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #250      +/-   ##
==========================================
+ Coverage   81.31%   81.36%   +0.05%     
==========================================
  Files         156      158       +2     
  Lines        7503     7733     +230     
  Branches     1102     1145      +43     
==========================================
+ Hits         6101     6292     +191     
- Misses       1271     1297      +26     
- Partials      131      144      +13     
Flag Coverage Δ
unittests 81.36% <83.40%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
mmedit/models/restorers/basicvsr.py 73.49% <73.49%> (ø)
mmedit/core/evaluation/metrics.py 94.88% <87.50%> (-0.64%) ⬇️
...edit/models/backbones/sr_backbones/basicvsr_net.py 88.54% <88.54%> (ø)
mmedit/models/backbones/__init__.py 100.00% <100.00%> (ø)
mmedit/models/backbones/sr_backbones/__init__.py 100.00% <100.00%> (ø)
mmedit/models/restorers/__init__.py 100.00% <100.00%> (ø)

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 9243ffc...1dd1009. Read the comment docs.

@innerlee innerlee merged commit 1cfce3a into open-mmlab:master Apr 12, 2021
@ckkelvinchan ckkelvinchan deleted the metric branch April 14, 2021 10:41
Yshuo-Li pushed a commit to Yshuo-Li/mmediting that referenced this pull request Jul 15, 2022
* Support Y-channel PSNR and SSIM

* fix bug

* update naming and unittest
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.

2 participants