Fix bug in SSIM #213
Fix bug in SSIM #213
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Ready for review. |
| return msssim_val | ||
| ssim_val = ss.mean(dim=(-1, -2)) | ||
| cs = cs.mean(dim=(-1, -2)) | ||
| return ssim_val, cs | ||
|
|
||
|
|
||
| def _ssim_per_channel_complex(x: torch.Tensor, y: torch.Tensor, kernel: torch.Tensor, |
There was a problem hiding this comment.
Shouldn't we update the complex-valued SSIM similarly to SSIM?
| k1=self.k1, k2=self.k2) | ||
| return torch.ones_like(score) - score | ||
|
|
||
|
|
||
| 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]]: |
There was a problem hiding this comment.
In case of avoiding Unions as much as possible, we should replace it with List[torch.Tensor].
|
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 Let me know if you think different. Otherwise I will modify Sonar rules to ignore duplications in tests and coverage in |
Signed-off-by: Sergey Kastryulin <snk4tr@gmail.com>
Signed-off-by: Sergey Kastryulin <snk4tr@gmail.com>
|
@snk4tr disagree. |
|
@denproc what do you think? |
|
@zakajd are you working on the one? Let's finilize this one. Let me know if you need any help. |
|
@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). |
|
Kudos, SonarCloud Quality Gate passed!
|
|
@zakajd are you still working on this one or it is ready? |
|
@snk4tr it's ready |
There was a problem hiding this comment.
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 😉
Closes #209 and closes #203
Proposed Changes
TODO: