[metrics] Update SSIM #4566

ydcjeff · 2020-11-07T13:20:31Z

What does this PR do?

Update SSIM and tests to compare with lower absolute tolerance

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In in short, see following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

teddykoker · 2020-11-07T21:05:43Z

Thanks for working on this!

pep8speaks · 2020-11-09T15:54:52Z

Hello @ydcjeff! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-19 01:34:41 UTC

codecov · 2020-11-10T08:13:34Z

Codecov Report

Merging #4566 (9394d81) into master (7383a2c) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4566   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         117     117           
  Lines        8957    8963    +6     
======================================
+ Hits         8324    8330    +6     
  Misses        633     633

ydcjeff · 2020-11-10T08:40:46Z

@teddykoker @SkafteNicki mind review a bit so far?
It has now squashed to 7e-5 tolerance

tchaton

LGTM ! I had a doubt with F.pad, but it is correct usage.

tests/metrics/regression/test_ssim.py

Co-authored-by: chaton <thomas@grid.ai>

Borda

lgtm, but I am not a metrics guy 🐰

pytorch_lightning/metrics/functional/ssim.py

teddykoker · 2020-11-16T19:50:18Z

tests/metrics/regression/test_ssim.py

@@ -50,7 +51,7 @@ def _sk_metric(preds, target, data_range, multichannel):
    [(i.preds, i.target, i.multichannel) for i in _inputs],
 )
 class TestSSIM(MetricTester):
-    atol = 1e-3  # TODO: ideally tests should pass with lower tolerance
+    atol = 6e-5

    # TODO: for some reason this test hangs with ddp=True
    # @pytest.mark.parametrize("ddp", [True, False])


Do tests pass with ddp=True?

The test hangs with ddp=True

@ydcjeff I just tried locally and the test does not hang for me when ddp=True.

When I tried before, the test hangs on drone, will try again.

Okay test still hangs on drone, so lets just disable ddp. Really not sure why it is not working.

pytorch_lightning/metrics/functional/ssim.py

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

teddykoker

DDP tests hang locally for me, but I guess its fine

* [metrics] Update SSIM * [metrics] Update SSIM * [metrics] Update SSIM * [metrics] Update SSIM * [metrics] update ssim * dist_sync_on_step True * [metrics] update ssim * Update tests/metrics/regression/test_ssim.py Co-authored-by: chaton <thomas@grid.ai> * Update pytorch_lightning/metrics/functional/ssim.py Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> * ddp=True * Update test_ssim.py Co-authored-by: chaton <thomas@grid.ai> Co-authored-by: Sean Naren <sean.narenthiran@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>

* [metrics] Update SSIM * [metrics] Update SSIM * [metrics] Update SSIM * [metrics] Update SSIM * [metrics] update ssim * dist_sync_on_step True * [metrics] update ssim * Update tests/metrics/regression/test_ssim.py Co-authored-by: chaton <thomas@grid.ai> * Update pytorch_lightning/metrics/functional/ssim.py Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> * ddp=True * Update test_ssim.py Co-authored-by: chaton <thomas@grid.ai> Co-authored-by: Sean Naren <sean.narenthiran@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Nicki Skafte <skaftenicki@gmail.com> (cherry picked from commit c361213)

[metrics] Update SSIM

51870e3

ydcjeff added bug Something isn't working Metrics labels Nov 7, 2020

ydcjeff added this to the 1.0.x milestone Nov 7, 2020

ydcjeff requested review from ananyahjha93, awaelchli, Borda, justusschock, nateraw, SeanNaren, tchaton, teddykoker and williamFalcon as code owners November 7, 2020 13:20

ydcjeff marked this pull request as draft November 7, 2020 13:20

Merge remote-tracking branch 'origin/master' into metrics/ssim

e582d33

ydcjeff added 4 commits November 9, 2020 17:42

Merge remote-tracking branch 'origin/master' into metrics/ssim

6e122df

Merge remote-tracking branch 'origin/master' into metrics/ssim

a800ab1

Merge remote-tracking branch 'origin/master' into metrics/ssim

12ede93

[metrics] Update SSIM

f5fbd5f

ydcjeff added 5 commits November 9, 2020 22:28

Merge remote-tracking branch 'origin/master' into metrics/ssim

5b3f2f0

[metrics] Update SSIM

a06ffa2

[metrics] Update SSIM

89ca5ce

Merge remote-tracking branch 'origin/master' into metrics/ssim

41c0917

[metrics] update ssim

2acfae9

dist_sync_on_step True

ed2a992

Jeff Yang and others added 2 commits November 14, 2020 08:21

Merge branch 'master' into metrics/ssim

3ea91a4

Merge branch 'master' into metrics/ssim

c89bfb8

tchaton approved these changes Nov 16, 2020

View reviewed changes

tests/metrics/regression/test_ssim.py Outdated Show resolved Hide resolved

Jeff Yang and others added 2 commits November 16, 2020 19:03

Update tests/metrics/regression/test_ssim.py

4fd0e2e

Co-authored-by: chaton <thomas@grid.ai>

Merge branch 'master' into metrics/ssim

543162d

rohitgr7 approved these changes Nov 16, 2020

View reviewed changes

Borda reviewed Nov 16, 2020

View reviewed changes

pytorch_lightning/metrics/functional/ssim.py Outdated Show resolved Hide resolved

Merge branch 'master' into metrics/ssim

5b8dcc8

teddykoker suggested changes Nov 16, 2020

View reviewed changes

Jeff Yang and others added 4 commits November 17, 2020 08:14

Update pytorch_lightning/metrics/functional/ssim.py

af29e9b

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Merge branch 'master' into metrics/ssim

d1f5eff

ddp=True

350faca

Update test_ssim.py

a653689

Borda requested a review from teddykoker November 17, 2020 23:44

Jeff Yang added 2 commits November 18, 2020 13:09

Merge branch 'master' into metrics/ssim

3b560fd

Merge branch 'master' into metrics/ssim

7f2721f

Borda modified the milestones: 1.0.x, 1.1 Nov 18, 2020

teddykoker approved these changes Nov 18, 2020

View reviewed changes

SkafteNicki and others added 3 commits November 18, 2020 20:51

Merge branch 'master' into metrics/ssim

b9289c1

Merge branch 'master' into metrics/ssim

65a361e

Merge branch 'master' into metrics/ssim

9394d81

ydcjeff merged commit c361213 into master Nov 19, 2020

ydcjeff deleted the metrics/ssim branch November 19, 2020 05:21

Borda modified the milestones: 1.1, 1.0.x Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[metrics] Update SSIM #4566

[metrics] Update SSIM #4566

ydcjeff commented Nov 7, 2020 •

edited

Loading

teddykoker commented Nov 7, 2020

pep8speaks commented Nov 9, 2020 •

edited

Loading

codecov bot commented Nov 10, 2020 •

edited

Loading

ydcjeff commented Nov 10, 2020

tchaton left a comment

Borda left a comment

teddykoker Nov 16, 2020

ydcjeff Nov 17, 2020

SkafteNicki Nov 17, 2020

ydcjeff Nov 17, 2020

SkafteNicki Nov 17, 2020

teddykoker left a comment

[metrics] Update SSIM #4566

[metrics] Update SSIM #4566

Conversation

ydcjeff commented Nov 7, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

teddykoker commented Nov 7, 2020

pep8speaks commented Nov 9, 2020 • edited Loading

Comment last updated at 2020-11-19 01:34:41 UTC

codecov bot commented Nov 10, 2020 • edited Loading

Codecov Report

ydcjeff commented Nov 10, 2020

tchaton left a comment

Choose a reason for hiding this comment

Borda left a comment

Choose a reason for hiding this comment

teddykoker Nov 16, 2020

Choose a reason for hiding this comment

ydcjeff Nov 17, 2020

Choose a reason for hiding this comment

SkafteNicki Nov 17, 2020

Choose a reason for hiding this comment

ydcjeff Nov 17, 2020

Choose a reason for hiding this comment

SkafteNicki Nov 17, 2020

Choose a reason for hiding this comment

teddykoker left a comment

Choose a reason for hiding this comment

ydcjeff commented Nov 7, 2020 •

edited

Loading

pep8speaks commented Nov 9, 2020 •

edited

Loading

codecov bot commented Nov 10, 2020 •

edited

Loading