PSNR implementation #2483

InCogNiTo124 · 2020-07-03T08:47:28Z

What does this PR do?

Implements a new metric: PSNR (peak signal to noise ratio)

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.
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 🙃

pep8speaks · 2020-07-03T08:47:31Z

Hello @InCogNiTo124! Thanks for updating this PR.

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

Comment last updated at 2020-07-08 08:24:51 UTC

Borda

let's add a test and compare it to some standard lib like https://scikit-image.org/docs/stable/api/skimage.measure.html#compare-psnr

justusschock · 2020-07-03T14:14:26Z

pytorch_lightning/metrics/regression.py

+        # Since mean and variance are unknown, we cannot know what's the maximum value to use in calculation.
+        # This implementation, therefore, finds the maximum empirically.
+        maximum = max(torch.max(torch.abs(pred)), torch.max(torch.abs(target)))
+        PSNR_base_e = 2*torch.log(maximum) - torch.log(mse)


variable names should always be lowercase according to pep8

InCogNiTo124 · 2020-07-03T21:47:47Z

I've added tests and I validated them with skimage.metrics.peak_signal_noise_ratio, and it works (even the example in the comments)

Borda · 2020-07-03T22:09:50Z

I've added tests and I validated them with skimage.metrics.peak_signal_noise_ratio, and it works (even the example in the comments)

add such validation test with skimage to our tests 🐰

InCogNiTo124 · 2020-07-03T22:28:31Z

I've added tests and I validated them with skimage.metrics.peak_signal_noise_ratio, and it works (even the example in the comments)

add such validation test with skimage to our tests

I wanted to, but I came to the conclusion that skimage is not in this project's requirements (I've installed the devel file) and I didn't want to change them :) I'll edit it tommorow then

awaelchli · 2020-07-03T23:50:18Z

I agree with @Borda that checking with the skimage implementation for maybe random tensors would be the best.
Could we also add a functional interface and coordinate with #2492 ?

Borda · 2020-07-03T23:52:01Z

it is fine to add skimage to requirements/test.txt :]

rohitgr7 · 2020-07-04T11:19:02Z

You should add PSNR in this file too :)

InCogNiTo124 · 2020-07-04T15:24:03Z

I agree with @Borda that checking with the skimage implementation for maybe random tensors would be the best.

I'm having trouble figuring out how to test with random inputs, I don't know how do I pass randomly generated tensors from pytest.param into skimage.metrics.peak_signal_noise_ratio.

Could we also add a functional interface and coordinate with #2492 ?

Sure :D

codecov · 2020-07-04T21:11:22Z

Codecov Report

Merging #2483 into master will decrease coverage by 2%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #2483    +/-   ##
=======================================
- Coverage      89%     87%    -2%     
=======================================
  Files          69      70     +1     
  Lines        5518    5648   +130     
=======================================
+ Hits         4889    4919    +30     
- Misses        629     729   +100

rohitgr7 · 2020-07-04T21:26:14Z

Here also you should add PSNR :] https://github.com/PyTorchLightning/pytorch-lightning/blob/39a6435726e7a110df1189efe63094888e0c23be/pytorch_lightning/metrics/__init__.py#L49-L54

InCogNiTo124 · 2020-07-04T21:33:52Z

Here also you should add PSNR :]

https://github.com/PyTorchLightning/pytorch-lightning/blob/39a6435726e7a110df1189efe63094888e0c23be/pytorch_lightning/metrics/__init__.py#L49-L54

I totally missed it, it should be fixed now :)

rohitgr7 · 2020-07-04T21:39:37Z

pytorch_lightning/metrics/regression.py

+        if self.data_range is None:
+            data_range = max(target.max() - target.min(), pred.max() - pred.min())
+        else:
+            data_range = torch.tensor(float(self.data_range))
+        mse = F.mse_loss(pred.view(-1), target.view(-1))
+        # numerical precision tricks
+        psnr_base_e = 2 * torch.log(data_range) - torch.log(mse)
+        return psnr_base_e * (10 / torch.log(self.base))  # change the logarithm basis


You don't have to replicate the whole procedure again here. Just import from functional and use that one. :) Also add reduction parameter here.

Refactored and added

BTW in case you didn't know, pytest says 'elementwise_mean' is deprecated, it might be smart to refactor the default to 'mean'. I left it that way to match rohitgr7's PR.

Nice! Also, can you fix the pep8 issues mentioned above by @pep8speaks.

@rohitgr7 I'm confused about the missing whitespace. I thought that was a bad thing :|

hmm, don't know about that :| I never use an extra , after the last element in the list.

Nvm I figured what was wrong

InCogNiTo124 · 2020-07-04T23:23:04Z

I've got a few failing tests I don't know how to fix:

I have no idea why ufunc is giving troubles in windows builds
conda tests are missing skimage (weird because I've added in test requirements)
there is a weird docker tag

awaelchli · 2020-07-04T23:26:46Z

conda tests are missing skimage (weird because I've added in test requirements)

@InCogNiTo124 I'm pretty sure you need to add it also to environment.yml

awaelchli · 2020-07-04T23:29:10Z

I have no idea why ufunc is giving troubles in windows builds

Lift numpy to 1.16.4 in requirements/base.txt, this was fixed in this version.
Source: scikit-learn-contrib/hdbscan#272

pytorch_lightning/metrics/functional/regression.py

SkafteNicki · 2020-07-06T15:08:48Z

If you could add a test similar to https://github.com/PyTorchLightning/pytorch-lightning/blob/a91b06ed1e5295efecdd7b51f4a3e6d95c829ecd/tests/metrics/functional/test_classification.py#L38-L72

to test on random tensors:

create two random tensors inside test
calculate value using skimage implementation and your implementation
assert results are same

InCogNiTo124 · 2020-07-06T16:20:48Z

@SkafteNicki you mean alongside or instead of these tests I already wrote?

SkafteNicki · 2020-07-06T16:23:46Z

Just add a new test :)

InCogNiTo124 · 2020-07-06T17:58:58Z

I googled the error in the TPU test, it seems that PROJECT_ID is not set.

Borda · 2020-07-06T18:16:55Z

I googled the error in the TPU test, it seems that PROJECT_ID is not set.

Pls ignore TPU test for this moment, the issue is related to forked PRs... cc: @zcain117

InCogNiTo124 · 2020-07-06T18:22:33Z

Pls ignore TPU test for this moment, the issue is related to forked PRs... cc: @zcain117

In that case, this seems done then :)

SkafteNicki · 2020-07-06T20:21:33Z

Could you update CHANGELOG?
Also we need refer to this in the docs, but maybe we should just merge this and then fix that in #2492?

Borda

LGTM :]

SkafteNicki

LGTM :)

CHANGELOG.md

Add stub PSNR metric

2a65f51

mergify bot requested a review from a team July 3, 2020 08:48

Borda added the feature Is an improvement or enhancement label Jul 3, 2020

Borda requested changes Jul 3, 2020

View reviewed changes

mergify bot requested a review from a team July 3, 2020 10:33

justusschock reviewed Jul 3, 2020

View reviewed changes

mergify bot requested a review from a team July 3, 2020 14:15

InCogNiTo124 added 3 commits July 3, 2020 22:35

Fix linter

d964e7f

Add data range as parameter

9f7f6b0

Add tests

a6e27e0

InCogNiTo124 changed the title ~~[WIP] PSNR implementation~~ PSNR implementation Jul 3, 2020

Add scikit-image

bcecb53

Add PSNR to regression metrics and add functional

fcd425f

rohitgr7 suggested changes Jul 4, 2020

View reviewed changes

InCogNiTo124 added 5 commits July 5, 2020 00:01

Refactor to functional

2fd9811

Fix linter

b4a2193

Fix linter, again

3cba8d5

Fix linter, again

0fdf13b

Fix typo in test

23db458

Add scikit-image to conda

7f6cee8

awaelchli reviewed Jul 4, 2020

View reviewed changes

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

mergify bot requested a review from a team July 4, 2020 23:31

Lift numpy requirement

b32beb6

Add random tests

dbbb4b9

Update CHANGELOG.md

552f9da

Borda approved these changes Jul 7, 2020

View reviewed changes

mergify bot requested a review from a team July 7, 2020 08:00

Borda added this to the 0.8.x milestone Jul 7, 2020

justusschock approved these changes Jul 7, 2020

View reviewed changes

mergify bot requested a review from a team July 7, 2020 09:03

SkafteNicki approved these changes Jul 7, 2020

View reviewed changes

Borda requested a review from awaelchli July 8, 2020 08:23

Borda reviewed Jul 8, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Apply suggestions from code review

7be6334

Borda merged commit 1dc7242 into Lightning-AI:master Jul 8, 2020

SkafteNicki mentioned this pull request Jul 8, 2020

Add functional regression metrics #2492

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSNR implementation #2483

PSNR implementation #2483

InCogNiTo124 commented Jul 3, 2020 •

edited

Loading

pep8speaks commented Jul 3, 2020 •

edited

Loading

Borda left a comment •

edited

Loading

justusschock Jul 3, 2020

InCogNiTo124 Jul 3, 2020

InCogNiTo124 commented Jul 3, 2020

Borda commented Jul 3, 2020

InCogNiTo124 commented Jul 3, 2020 •

edited

Loading

awaelchli commented Jul 3, 2020

Borda commented Jul 3, 2020

rohitgr7 commented Jul 4, 2020

InCogNiTo124 commented Jul 4, 2020

codecov bot commented Jul 4, 2020 •

edited

Loading

rohitgr7 commented Jul 4, 2020

InCogNiTo124 commented Jul 4, 2020

rohitgr7 Jul 4, 2020

InCogNiTo124 Jul 4, 2020

rohitgr7 Jul 4, 2020

InCogNiTo124 Jul 4, 2020

rohitgr7 Jul 4, 2020

InCogNiTo124 Jul 4, 2020

InCogNiTo124 commented Jul 4, 2020

awaelchli commented Jul 4, 2020

awaelchli commented Jul 4, 2020 •

edited

Loading

SkafteNicki commented Jul 6, 2020

InCogNiTo124 commented Jul 6, 2020

SkafteNicki commented Jul 6, 2020

InCogNiTo124 commented Jul 6, 2020

Borda commented Jul 6, 2020

InCogNiTo124 commented Jul 6, 2020

SkafteNicki commented Jul 6, 2020

Borda left a comment

SkafteNicki left a comment

PSNR implementation #2483

PSNR implementation #2483

Conversation

InCogNiTo124 commented Jul 3, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Jul 3, 2020 • edited Loading

Comment last updated at 2020-07-08 08:24:51 UTC

Borda left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InCogNiTo124 commented Jul 3, 2020

Borda commented Jul 3, 2020

InCogNiTo124 commented Jul 3, 2020 • edited Loading

awaelchli commented Jul 3, 2020

Borda commented Jul 3, 2020

rohitgr7 commented Jul 4, 2020

InCogNiTo124 commented Jul 4, 2020

codecov bot commented Jul 4, 2020 • edited Loading

Codecov Report

rohitgr7 commented Jul 4, 2020

InCogNiTo124 commented Jul 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InCogNiTo124 commented Jul 4, 2020

awaelchli commented Jul 4, 2020

awaelchli commented Jul 4, 2020 • edited Loading

SkafteNicki commented Jul 6, 2020

InCogNiTo124 commented Jul 6, 2020

SkafteNicki commented Jul 6, 2020

InCogNiTo124 commented Jul 6, 2020

Borda commented Jul 6, 2020

InCogNiTo124 commented Jul 6, 2020

SkafteNicki commented Jul 6, 2020

Borda left a comment

Choose a reason for hiding this comment

SkafteNicki left a comment

Choose a reason for hiding this comment

InCogNiTo124 commented Jul 3, 2020 •

edited

Loading

pep8speaks commented Jul 3, 2020 •

edited

Loading

Borda left a comment •

edited

Loading

InCogNiTo124 commented Jul 3, 2020 •

edited

Loading

codecov bot commented Jul 4, 2020 •

edited

Loading

awaelchli commented Jul 4, 2020 •

edited

Loading