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

3D extension for SSIM #818

Merged
merged 125 commits into from
Mar 25, 2022
Merged

3D extension for SSIM #818

merged 125 commits into from
Mar 25, 2022

Conversation

weningerleon
Copy link
Contributor

@weningerleon weningerleon commented Jan 31, 2022

What does this PR do?

Fixes #812
Changes StructuralSimilarityIndexMeasure to be able to handle 3D images. Deciding whether the 2D or 3D version is used is determined automatically based on kernel_size. Basic sanity checks are carried out. Produces sensible results on my dataset.

Before submitting

  • Was this discussed/approved 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 🙃

Copy link
Member

@justusschock justusschock left a comment

Choose a reason for hiding this comment

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

Some first comments.

Usually there are 2 ways for testing:

1.) Use random data and compare the results with a gold-standard implementation (like sklearn)

2.) Use predefined-data, calculate result manually once and hardcode those results as targets.

Ideally we would prefer Option 1 since that way you can always test it with additional data if required. I think the sklearn implementation here should also work with 3D data :)

Edit: have a look at the 2d tests here

torchmetrics/functional/image/ssim.py Outdated Show resolved Hide resolved
torchmetrics/functional/image/ssim.py Outdated Show resolved Hide resolved
torchmetrics/functional/image/ssim.py Outdated Show resolved Hide resolved
torchmetrics/functional/image/ssim.py Outdated Show resolved Hide resolved
torchmetrics/functional/image/ssim.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #818 (b057212) into master (25c4b90) will decrease coverage by 0%.
The diff coverage is 88%.

@@          Coverage Diff          @@
##           master   #818   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         173    173           
  Lines        7257   7328   +71     
=====================================
+ Hits         6900   6958   +58     
- Misses        357    370   +13     

@weningerleon
Copy link
Contributor Author

weningerleon commented Feb 1, 2022

I think your current tests that compare the results from torchmetrics with the results from skimage are faulty.
Have a look at the 2d tests mentioned by @justusschock, function _sk_ssim:
You permute the 4D tensor (BxCxHxW) to BxHxWxC, with B=batch_size, C=image channels. Skimage treats the BxHxWxC tensor as a 3D image with C channels. However, it needs to be treated as B 2D images (skimage does not support batching). This also explains why you need to set atol to 6e-3, which is really high.

@justusschock
Copy link
Member

@weningerleon If you could fix those while working on it, this would also be highly appreciated :D

@weningerleon
Copy link
Contributor Author

@weningerleon If you could fix those while working on it, this would also be highly appreciated :D

Yes, have a look at the updates files ;)

@justusschock justusschock mentioned this pull request Feb 5, 2022
4 tasks
@mergify mergify bot removed the has conflicts label Mar 20, 2022
@justusschock
Copy link
Member

@Borda why are the tests not running?

@Borda
Copy link
Member

Borda commented Mar 24, 2022

@Borda why are the tests not running?

I guess it is due to GH outage...

@Borda
Copy link
Member

Borda commented Mar 24, 2022

@justusschock mind check the failing tests, pls? 🐰

@Borda Borda enabled auto-merge (squash) March 24, 2022 14:50
@Borda Borda dismissed stancld’s stale review March 25, 2022 01:09

talked to him and he is fine with passing tests :)

@Borda Borda merged commit d85c943 into Lightning-AI:master Mar 25, 2022
@mergify mergify bot added the ready label Mar 25, 2022
@Borda
Copy link
Member

Borda commented Mar 25, 2022

@weningerleon thank you for all your effort; very appreciated!

@weningerleon
Copy link
Contributor Author

Glad I could help to improve this awesome project!
Thanks a lot for your support!

@Borda Borda added this to the v0.8 milestone May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3D SSIM
5 participants