[Metrics] Unification of regression #4166

SkafteNicki · 2020-10-15T08:52:05Z

What does this PR do?

Attempt at unification between the new class based regression metrics and their functional counterpart.
In short, each functional now consist of three functions:

def _metric_name_update(...)
def _metric_name_compute(...)
def mean_name(...):

where the two first we import into the corresponding class based metric and use in the respective update and compute method. The last is what the user interacts with if they use the class based interface.
I bit of renaming is also happening (again for consistency) and removal of old redundant tests.
Tagging @ananyahjha93 and @teddykoker for opinion.

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-10-15T08:52:12Z

Hello @SkafteNicki! Thanks for updating this PR.

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

Comment last updated at 2020-10-21 20:35:36 UTC

justusschock

I really like what you did here. I added some suggestions on typing, but the code itself is fine.

My only concern is about tests...

pytorch_lightning/metrics/functional/explained_variance.py

pytorch_lightning/metrics/functional/mean_absolute_error.py

pytorch_lightning/metrics/functional/mean_squared_log_error.py

tests/metrics/functional/test_regression.py

Co-authored-by: Justus Schock <[email protected]>

teddykoker

Excellent! This should be the standard for metrics moving forward :)

…lightning into metrics/unification_regression

teddykoker · 2020-10-20T00:12:02Z

Can probably change the rest of the regression metrics to have _compute and _update function. (even if the update function does nothing for now)

teddykoker · 2020-10-20T00:12:27Z

Also the plan would be to do this with the classification metrics too right?

codecov · 2020-10-20T00:34:27Z

Codecov Report

Merging #4166 into master will increase coverage by 3%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #4166    +/-   ##
=======================================
+ Coverage      90%     93%    +3%     
=======================================
  Files         103     109     +6     
  Lines        7842    7912    +70     
=======================================
+ Hits         7053    7349   +296     
+ Misses        789     563   -226

…d error

teddykoker · 2020-10-20T22:55:42Z

Currently SSIM is failing tests. When it was originally added it was only passing due to a tolerance of 1e-4 which is still failing now with the new more rigorous tests. @ydcjeff do you think you could take a look?

ydcjeff · 2020-10-21T03:04:32Z

Sure, I will take a look.

SkafteNicki · 2020-10-21T07:35:36Z

@teddykoker agree, that we should do the same unification for all the classification metrics. There are just more of them so I started the easy once 😄

justusschock · 2020-10-21T09:12:13Z

From the code so far, I can approve this PR (except for failing test of course :D)

teddykoker · 2020-10-21T14:32:13Z

@justusschock @SkafteNicki All of the regression metrics are unified now; only issue is that the structural similarity tests are failing. Do you think we should just comment them out and they can be fixed in a new PR? would be great if we could get this merged ASAP so we can start doing the same with the classification metrics

ydcjeff · 2020-10-21T14:45:13Z

The results for SSIM are matching up only to 4 or 5 decimal points, I am not sure how to match up to 6 or 7 decimal points

teddykoker · 2020-10-21T15:51:11Z

@ydcjeff I increased the tolerance for ssim tests to 1e-3, should pass all tests now. Would be great if we could lower the tolerance in the future, there seems to be a few differences between the skimage and our implementation

Borda · 2020-10-22T08:08:09Z

pytorch_lightning/metrics/regression/ssim.py

+            process_group=process_group,
+        )
+        rank_zero_warn(
+            'Metric `SSIM` will save all targets and'


we can use 120-length lines

Borda · 2020-10-22T08:19:49Z

tests/metrics/regression/test_psnr.py

+    return peak_signal_noise_ratio(sk_target, sk_preds, data_range=data_range)
+
+
+def _base_e_sk_metric(preds, target, data_range):


what is the _base_E_sk_metric for?

that is for testing when the base parameter in our own implementation is different from log-base-10 (in this case log-e, i.e. the natural logarithm)

SkafteNicki added 6 commits October 15, 2020 09:47

moved to utility

82a414f

add files

555e232

unify

d4165f4

add desc

2659243

update

f990dbb

end of line

01edcbf

mergify bot requested a review from a team October 15, 2020 08:52

justusschock reviewed Oct 15, 2020

View reviewed changes

mergify bot requested a review from a team October 15, 2020 09:38

Borda and others added 4 commits October 15, 2020 12:05

Apply suggestions from code review

169a26c

Co-authored-by: Justus Schock <[email protected]>

Apply suggestions from code review

f27114c

Co-authored-by: Justus Schock <[email protected]>

add back functional test in new interface

99cb53a

pep8

4a79225

SkafteNicki added the Metrics label Oct 16, 2020

edenlightning added this to the 1.0.3 milestone Oct 19, 2020

edenlightning requested review from ananyahjha93 and teddykoker October 19, 2020 23:36

teddykoker suggested changes Oct 19, 2020

View reviewed changes

teddykoker added 2 commits October 19, 2020 20:04

Merge branch 'master' of https://github.com/PyTorchLightning/pytorch-…

753dec5

…lightning into metrics/unification_regression

doctest fix

abfa6ad

teddykoker approved these changes Oct 20, 2020

View reviewed changes

test name fix

52d6258

teddykoker changed the title ~~[Metrics] Unification of regression~~ [Metrics] Unification of regression [WIP] Oct 20, 2020

teddykoker added 2 commits October 19, 2020 20:59

unify psnr + add class psnr, TODO: psnr test refactor ala mean square…

acf736b

…d error

unify psnr

40246fb

Borda self-requested a review October 20, 2020 10:31

edenlightning modified the milestones: 1.0.3, 1.1 Oct 20, 2020

unify ssim

001e436

SkafteNicki mentioned this pull request Oct 21, 2020

Metrics do not support multilabel tasks. #4238

Closed

justusschock approved these changes Oct 21, 2020

View reviewed changes

mergify bot requested a review from a team October 21, 2020 09:11

lower tolerance for ssim

00e7d42

teddykoker added 4 commits October 21, 2020 11:53

fix import

1cd3982

pep8

55388b3

docs

ff648c1

flake8

ea95df6

teddykoker changed the title ~~[Metrics] Unification of regression [WIP]~~ [Metrics] Unification of regression Oct 21, 2020

teddykoker and others added 4 commits October 21, 2020 15:11

test smaller images

2acd996

trying to fix test

3f26164

no ddp test for ssim

9b96eba

pep8

849561f

ananyahjha93 approved these changes Oct 21, 2020

View reviewed changes

ananyahjha93 merged commit a937394 into Lightning-AI:master Oct 21, 2020

Borda reviewed Oct 22, 2020

View reviewed changes

SkafteNicki deleted the metrics/unification_regression branch October 22, 2020 08:09

Borda reviewed Oct 22, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metrics] Unification of regression #4166

[Metrics] Unification of regression #4166

SkafteNicki commented Oct 15, 2020

pep8speaks commented Oct 15, 2020 •

edited

Loading

justusschock left a comment

teddykoker left a comment

teddykoker commented Oct 20, 2020

teddykoker commented Oct 20, 2020

codecov bot commented Oct 20, 2020 •

edited

Loading

teddykoker commented Oct 20, 2020

ydcjeff commented Oct 21, 2020

SkafteNicki commented Oct 21, 2020

justusschock commented Oct 21, 2020

teddykoker commented Oct 21, 2020

ydcjeff commented Oct 21, 2020

teddykoker commented Oct 21, 2020

Borda Oct 22, 2020

Borda Oct 22, 2020

SkafteNicki Oct 22, 2020

		return peak_signal_noise_ratio(sk_target, sk_preds, data_range=data_range)


		def _base_e_sk_metric(preds, target, data_range):

[Metrics] Unification of regression #4166

[Metrics] Unification of regression #4166

Conversation

SkafteNicki commented Oct 15, 2020

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Oct 15, 2020 • edited Loading

Comment last updated at 2020-10-21 20:35:36 UTC

justusschock left a comment

Choose a reason for hiding this comment

teddykoker left a comment

Choose a reason for hiding this comment

teddykoker commented Oct 20, 2020

teddykoker commented Oct 20, 2020

codecov bot commented Oct 20, 2020 • edited Loading

Codecov Report

teddykoker commented Oct 20, 2020

ydcjeff commented Oct 21, 2020

SkafteNicki commented Oct 21, 2020

justusschock commented Oct 21, 2020

teddykoker commented Oct 21, 2020

ydcjeff commented Oct 21, 2020

teddykoker commented Oct 21, 2020

Borda Oct 22, 2020

Choose a reason for hiding this comment

Borda Oct 22, 2020

Choose a reason for hiding this comment

SkafteNicki Oct 22, 2020

Choose a reason for hiding this comment

pep8speaks commented Oct 15, 2020 •

edited

Loading

codecov bot commented Oct 20, 2020 •

edited

Loading