Fix bug in SSIM by jzakirov · Pull Request #213 · photosynthesis-team/piq

jzakirov · 2021-01-21T21:04:51Z

Closes #209 and closes #203

Proposed Changes

Split code for SSIM and MS-SSIM
Fix bug (which ocured to to missing average pooling. See skimage.metrics.structural_similarity() vs MATLAB scikit-image/scikit-image#5192 for details
Rename variables to have a more meaningful names

TODO:

Add tests to compare with MATLAB
Update table in README

codecov · 2021-01-22T09:15:54Z

Codecov Report

Merging #213 (051b5a7) into master (d401e8c) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   95.18%   95.21%   +0.03%     
==========================================
  Files          28       29       +1     
  Lines        1766     1778      +12     
==========================================
+ Hits         1681     1693      +12     
  Misses         85       85

Flag	Coverage Δ
unittests	`95.21% <100.00%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
piq/__init__.py	`100.00% <100.00%> (ø)`
piq/ms_ssim.py	`100.00% <100.00%> (ø)`
piq/ssim.py	`100.00% <100.00%> (ø)`

jzakirov · 2021-01-22T09:38:54Z

Ready for review.
Test failed due to duplicate code in tests/test_ssim.py and tests/test_ms_ssim.py. This will be fixed during test refactoring (#97 )

denproc

Nice catch with downsampling bug!
Minor comments about the documentation is to be resolved. Others are optional and can be discussed.

piq/ms_ssim.py

denproc · 2021-01-26T09:03:55Z

piq/ssim.py

+    return ssim_val, cs


 def _ssim_per_channel_complex(x: torch.Tensor, y: torch.Tensor, kernel: torch.Tensor,


Shouldn't we update the complex-valued SSIM similarly to SSIM?

denproc · 2021-01-26T09:14:12Z

piq/ssim.py

-
 def _ssim_per_channel(x: torch.Tensor, y: torch.Tensor, kernel: torch.Tensor,
                      data_range: Union[float, int] = 1., k1: float = 0.01,
                      k2: float = 0.03) -> Union[torch.Tensor, Tuple[torch.Tensor, torch.Tensor]]:


In case of avoiding Unions as much as possible, we should replace it with List[torch.Tensor].

snk4tr · 2021-01-27T18:22:08Z

Hey guys. Sonar check fails because of code duplication. As far as I can see, all duplications are in tests, which is absolutely fine with me. I will also remove piq/feature_extractors from the coverage report since it contains standard feature extraction model architecture that is not means to be tested.

Let me know if you think different. Otherwise I will modify Sonar rules to ignore duplications in tests and coverage in piq/feature_extractors.

Signed-off-by: Sergey Kastryulin <snk4tr@gmail.com>

jzakirov · 2021-01-27T21:09:00Z

@snk4tr disagree.
Let's leave Sonar cloud checks for feature extractors and add proper testing for models instead. At least check that forward call runs without failure.
We can also move new models there (like PieAPP)

snk4tr · 2021-01-28T07:48:11Z

@denproc what do you think?

snk4tr · 2021-01-30T08:41:14Z

@denproc what do you think?

Ok, I see that @denproc liked the idea of not removing feature extractors from the coverage report. Reverted this change.

snk4tr · 2021-02-15T16:32:30Z

@zakajd are you working on the one? Let's finilize this one. Let me know if you need any help.

jzakirov · 2021-02-18T12:54:53Z

@snk4tr this one can be merged if there is no failing tests.

I didn't renamed variables in ms-ssim as proposed by Denis because its better to do that in a separate PR (along with some other renamings).

sonarqubecloud · 2021-02-26T11:15:40Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

snk4tr · 2021-03-01T18:29:13Z

@zakajd are you still working on this one or it is ready?

jzakirov · 2021-03-01T18:34:35Z

@snk4tr it's ready

snk4tr

The PR looks good, ready to merge.
@zakajd FYI: it is easier to follow PRs when they are make to solve a single issue. In this case, the PR is both about fixing the bug and refactoring, which confuses things a little. Consider making separate PRs for these two next time.
P.S. If you did that because of long review time, do not let long processes to break your style 😉

jzakirov added 3 commits January 21, 2021 13:44

refactoring/ssim: Split SSIM and MS-SSIM into separate files. Close #203

caa012e

bug/ssim: Add average pool to match performance with paper values

ac7e470

bug/ssim: Rename variables

b014f68

jzakirov requested a review from denproc January 21, 2021 21:05

jzakirov assigned denproc Jan 21, 2021

bug/ssim: MyPy and Flake8 compliance

6d4d301

bug/ssim: Add tests to compare with MATLAB. Make downsampling optional.

3a7be1b

jzakirov force-pushed the bug/ssim branch from 469b1d7 to 3a7be1b Compare January 22, 2021 09:16

denproc requested changes Jan 26, 2021

View reviewed changes

snk4tr added 4 commits January 27, 2021 21:23

Merge branch 'master' into bug/ssim

1e4198a

dummy commit to rerun sonar analysis

11efecd

Signed-off-by: Sergey Kastryulin <snk4tr@gmail.com>

dummy commit to rerun sonar analysis

733ebc1

Signed-off-by: Sergey Kastryulin <snk4tr@gmail.com>

Merge branch 'master' into bug/ssim

43852c3

jzakirov added 2 commits February 24, 2021 12:53

Merge branch 'master' into bug/ssim

8999e49

bug/ssim: Minor

667a6ae

jzakirov force-pushed the bug/ssim branch from 8517664 to 667a6ae Compare February 26, 2021 10:48

jzakirov added 2 commits February 26, 2021 13:53

Merge branch 'master' into bug/ssim

fdd50ce

big/ssim: Fix MS-SSIM and merge errors.

051b5a7

jzakirov force-pushed the bug/ssim branch from c220f66 to 051b5a7 Compare February 26, 2021 11:13

snk4tr approved these changes Mar 2, 2021

View reviewed changes

snk4tr merged commit 49c3663 into master Mar 2, 2021

snk4tr deleted the bug/ssim branch March 2, 2021 15:23

		return ssim_val, cs


		def _ssim_per_channel_complex(x: torch.Tensor, y: torch.Tensor, kernel: torch.Tensor,

Conversation

jzakirov commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

codecov bot commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jzakirov commented Jan 22, 2021

Uh oh!

denproc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

denproc Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

denproc Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

snk4tr commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jzakirov commented Jan 27, 2021

Uh oh!

snk4tr commented Jan 28, 2021

Uh oh!

snk4tr commented Jan 30, 2021

Uh oh!

snk4tr commented Feb 15, 2021

Uh oh!

jzakirov commented Feb 18, 2021

Uh oh!

sonarqubecloud bot commented Feb 26, 2021

Uh oh!

snk4tr commented Mar 1, 2021

Uh oh!

jzakirov commented Mar 1, 2021

Uh oh!

snk4tr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jzakirov commented Jan 21, 2021 •

edited

Loading

codecov bot commented Jan 22, 2021 •

edited

Loading

snk4tr commented Jan 27, 2021 •

edited

Loading