Skip to content

fix: MSSSIM and SSIM dist_reduce_fx when reduction=None for distributed training #3166

Merged
Borda merged 19 commits into
Lightning-AI:masterfrom
rittik9:main
Jul 4, 2025
Merged

fix: MSSSIM and SSIM dist_reduce_fx when reduction=None for distributed training #3166
Borda merged 19 commits into
Lightning-AI:masterfrom
rittik9:main

Conversation

@rittik9

@rittik9 rittik9 commented Jul 3, 2025

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes issue mentioned in #3159 (comment) also adds test for #3162.

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--3166.org.readthedocs.build/en/3166/

@Borda Borda left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we have a test for it?

@codecov

codecov Bot commented Jul 3, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70%. Comparing base (d3259c0) to head (ab401ea).
⚠️ Report is 77 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #3166    +/-   ##
=======================================
- Coverage      70%     70%    -0%     
=======================================
  Files         364     349    -15     
  Lines       20063   19868   -195     
=======================================
- Hits        14090   13896   -194     
+ Misses       5973    5972     -1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Borda Borda mentioned this pull request Jul 3, 2025
4 tasks
@rittik9 rittik9 changed the title Fix: MultiScaleStructuralSimilarityIndexMeasure dist_reduce_fx when reduction=None for distributed training Fix: MultiScaleStructuralSimilarityIndexMeasure dist_reduce_fx when reduction=None for distributed training[wip] Jul 3, 2025
@rittik9 rittik9 marked this pull request as draft July 3, 2025 19:25
@rittik9

rittik9 commented Jul 3, 2025

Copy link
Copy Markdown
Collaborator Author

Hi @Borda, altho I can reproduce the issue in my local with the reproducible code sample: #3159 provided. But here with the added test I am unable to reproduce the error in distributed setting. I need some help here to reproduce the issue. Then I can add the fix...

@Borda

Borda commented Jul 4, 2025

Copy link
Copy Markdown
Collaborator

Hi @Borda, altho I can reproduce the issue in my local with the reproducible code sample: #3159 provided. But here with the added test I am unable to reproduce the error in distributed setting. I need some help here to reproduce the issue. Then I can add the fix...

So, you say that the patch resolves the problem, but it is difficult to create a test for it?
In such a case, I trust you and I would merge it so we can think about the test later...

@Borda Borda changed the title Fix: MultiScaleStructuralSimilarityIndexMeasure dist_reduce_fx when reduction=None for distributed training[wip] test: MultiScaleStructuralSimilarityIndexMeasure dist_reduce_fx when reduction=None for distributed training [wip] Jul 4, 2025
@rittik9 rittik9 marked this pull request as ready for review July 4, 2025 09:42
@rittik9

rittik9 commented Jul 4, 2025

Copy link
Copy Markdown
Collaborator Author

finally able to reproduce the issue

@Borda Borda changed the title test: MultiScaleStructuralSimilarityIndexMeasure dist_reduce_fx when reduction=None for distributed training [wip] fix: MultiScaleStructuralSimilarityIndexMeasure dist_reduce_fx when reduction=None for distributed training [wip] Jul 4, 2025
@rittik9 rittik9 changed the title fix: MultiScaleStructuralSimilarityIndexMeasure dist_reduce_fx when reduction=None for distributed training [wip] fix: MSSSIM and SSIM dist_reduce_fx when reduction=None for distributed training Jul 4, 2025
@rittik9

rittik9 commented Jul 4, 2025

Copy link
Copy Markdown
Collaborator Author

Hi @Borda, this one ready for review. I've also added test for #3162.

@rittik9 rittik9 requested a review from Borda July 4, 2025 12:19
Comment thread tests/unittests/image/test_ms_ssim.py Outdated
@mergify mergify Bot added the ready label Jul 4, 2025
Comment thread tests/unittests/image/test_ms_ssim.py
@rittik9 rittik9 requested a review from Borda July 4, 2025 17:53
@Borda Borda merged commit 9c87924 into Lightning-AI:master Jul 4, 2025
62 checks passed
Borda pushed a commit that referenced this pull request Jul 4, 2025
…ibuted training (#3166)

* Update ssim.py
* Update test_ms_ssim.py

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 9c87924)
Borda pushed a commit that referenced this pull request Jul 5, 2025
…ibuted training (#3166)

* Update ssim.py
* Update test_ms_ssim.py

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 9c87924)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants