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

correct the padding related calculation errors in SSIM #2721

Merged
merged 15 commits into from
Sep 9, 2024

Conversation

petertheprocess
Copy link
Contributor

@petertheprocess petertheprocess commented Sep 4, 2024

What does this PR do?

Fixes #2718 ,
Before the convolution, we already applied same padding to the image

        preds = F.pad(preds, (pad_w, pad_w, pad_h, pad_h), mode="reflect")
        target = F.pad(target, (pad_w, pad_w, pad_h, pad_h), mode="reflect")

So ssim_idx_full_image will share the same shape as original image, there is no need to take the padding out again, otherwise it will result in losing information on edge.

    ssim_idx_full_image = ((2 * mu_pred_target + c1) * upper) / ((mu_pred_sq + mu_target_sq + c1) * lower)

    if is_3d:
        ssim_idx = ssim_idx_full_image[..., pad_h:-pad_h, pad_w:-pad_w, pad_d:-pad_d]
    else:
        ssim_idx = ssim_idx_full_image[..., pad_h:-pad_h, pad_w:-pad_w]

Comparison

 preds = torch.rand([3, 3, 256, 256])
 # let the edge of the image be 0
 target = preds.clone()
 target[:, :, 0, :] = 0
 target[:, :, -1, :] = 0
 target[:, :, :, 0] = 0
 target[:, :, :, -1] = 0
 print(structural_similarity_index_measure(preds, target))

Before this PR

tensor(1.0000), which should be wrong because the edge are different.

After this PR

tensor(0.9836), which make sense.

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃


📚 Documentation preview 📚: https://torchmetrics--2721.org.readthedocs.build/en/2721/

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

can we add test for this fix, pls

@SkafteNicki SkafteNicki added the bug / fix Something isn't working label Sep 5, 2024
@SkafteNicki SkafteNicki added this to the v1.4.x milestone Sep 5, 2024
@petertheprocess
Copy link
Contributor Author

Since we modified the calculation method, all previous quantitativ test will be affected.

Copy link

codecov bot commented Sep 7, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 38%. Comparing base (45a41b8) to head (7f97f07).
Report is 96 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (45a41b8) and HEAD (7f97f07). Click for more details.

HEAD has 160 uploads less than BASE
Flag BASE (45a41b8) HEAD (7f97f07)
Windows 8 3
python3.8 9 3
cpu 59 20
torch1.13.1+cpu 9 3
macOS 12 4
python3.10 15 5
torch2.0.1 6 2
python3.11 8 3
torch2.4.0 3 1
torch2.4.0+cpu 2 1
torch1.11.0+cpu 3 1
Linux 39 13
python3.9 27 9
torch1.12.1+cpu 3 1
torch1.10.2+cpu 3 1
torch2.0.1+cpu 9 3
torch1.13.1 3 1
torch2.3.1+cpu 6 2
torch2.2.2+cpu 6 2
torch2.1.2+cpu 3 1
torch2.4.0+cu121 3 1
gpu 2 0
unittest 2 0
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2721     +/-   ##
========================================
- Coverage      69%     38%    -31%     
========================================
  Files         329     316     -13     
  Lines       18064   17900    -164     
========================================
- Hits        12492    6761   -5731     
- Misses       5572   11139   +5567     

@mergify mergify bot added the ready label Sep 7, 2024
@SkafteNicki SkafteNicki merged commit efdb111 into Lightning-AI:master Sep 9, 2024
62 checks passed
Borda pushed a commit that referenced this pull request Sep 11, 2024
* correct the padding related calculation errors in SSIM

* fix doctests

* changelog

* change test

* add test

* fix syntax in test_ssim.py

* fix syntax in test_ssim.py

* fix unittests

* fix tolerance

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit efdb111)
Borda pushed a commit that referenced this pull request Sep 13, 2024
* correct the padding related calculation errors in SSIM

* fix doctests

* changelog

* change test

* add test

* fix syntax in test_ssim.py

* fix syntax in test_ssim.py

* fix unittests

* fix tolerance

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit efdb111)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working ready topic: Image
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSIM calculation mistake.
3 participants