[Metrics] class based embedding similarity + tests #3358

SkafteNicki · 2020-09-05T10:09:58Z

What does this PR do?

PR #3349 added a new functional metric embedding_similarity. This PR just follows up and adds a class based interface + adds tests against sklearn

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 🙃

docs/source/metrics.rst

codecov · 2020-09-06T03:01:55Z

Codecov Report

Merging #3358 into master will decrease coverage by 13%.
The diff coverage is 62%.

@@           Coverage Diff            @@
##           master   #3358     +/-   ##
========================================
- Coverage      90%     78%    -13%     
========================================
  Files          93     111     +18     
  Lines        8188   10286   +2098     
========================================
+ Hits         7397    7990    +593     
- Misses        791    2296   +1505

awaelchli

lgtm, just some whitespace that needs to be fixed for proper display in html

pytorch_lightning/metrics/self_supervised.py

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

pep8speaks · 2020-09-07T05:34:18Z

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-09-11 06:10:39 UTC

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

pytorch_lightning/metrics/__init__.py

pytorch_lightning/metrics/self_supervised.py

Borda · 2020-09-07T21:59:49Z

pytorch_lightning/metrics/metric.py

@@ -287,3 +287,6 @@ def output_convert(self, data: Any, output: Any):
    def ddp_sync(self, data: Any, output: Any):
        return apply_to_collection(output, torch.Tensor, sync_ddp_if_available,
                                   self.reduce_group, self.reduce_op)
+
+
+__all__ = ["Metric", "TensorMetric", "NumpyMetric"]


this is not a very common place for __all__

codefactor also complains about this, not sure why. I think it should be fine though.

the point is that it is very related to importing from packages when you do not want to import all functions
https://stackoverflow.com/questions/44834/can-someone-explain-all-in-python

I agree that it is not a common place for __all__, normally I would put it at the top of the file, but then codefactor complains about it. But I can move it to the top if that is better.

yes, it is not very common to have it in other files than __init__ so was there a reason to move it from the init?

I was changed due to a comment for justus at some point, but lets change it back since it is very uncommon practise

pytorch_lightning/metrics/self_supervised.py

SkafteNicki added 4 commits September 5, 2020 11:35

embedding similarity class + test

08791c0

fix tests

28f278a

fix pep8

a7cb2f4

add docs

ba9b421

mergify bot requested a review from a team September 5, 2020 10:10

SkafteNicki mentioned this pull request Sep 5, 2020

added emb similarity #3349

Merged

SkafteNicki added the Metrics label Sep 5, 2020

awaelchli reviewed Sep 6, 2020

View reviewed changes

docs/source/metrics.rst Outdated Show resolved Hide resolved

noindex

c96a5df

mergify bot requested a review from a team September 6, 2020 01:07

awaelchli reviewed Sep 6, 2020

View reviewed changes

docs/source/metrics.rst Outdated Show resolved Hide resolved

mergify bot requested a review from a team September 6, 2020 02:19

Update docs/source/metrics.rst

b9f17db

awaelchli approved these changes Sep 6, 2020

View reviewed changes

pytorch_lightning/metrics/self_supervised.py Outdated Show resolved Hide resolved

pytorch_lightning/metrics/self_supervised.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team September 6, 2020 03:45

rohitgr7 reviewed Sep 6, 2020

View reviewed changes

pytorch_lightning/metrics/self_supervised.py Outdated Show resolved Hide resolved

pytorch_lightning/metrics/self_supervised.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team September 6, 2020 16:52

Update pytorch_lightning/metrics/self_supervised.py

dd009c1

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

Update pytorch_lightning/metrics/self_supervised.py

9a7b342

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

justusschock reviewed Sep 7, 2020

View reviewed changes

pytorch_lightning/metrics/__init__.py Show resolved Hide resolved

pytorch_lightning/metrics/self_supervised.py Show resolved Hide resolved

mergify bot requested a review from a team September 7, 2020 05:39

SkafteNicki added 5 commits September 7, 2020 09:51

suggestions

345e383

changes to init

39824ba

merge + changelog

123b768

move __all__

299d2a8

fix imports

9832a8e

justusschock self-requested a review September 7, 2020 09:21

justusschock approved these changes Sep 7, 2020

View reviewed changes

mergify bot requested a review from a team September 7, 2020 09:21

rohitgr7 approved these changes Sep 7, 2020

View reviewed changes

Borda reviewed Sep 7, 2020

View reviewed changes

Apply suggestions from code review

78a8d37

awaelchli reviewed Sep 7, 2020

View reviewed changes

pytorch_lightning/metrics/self_supervised.py Outdated Show resolved Hide resolved

awaelchli and others added 2 commits September 8, 2020 00:31

assert typo

2e8fa6d

change import

366e835

Borda added the ready PRs ready to be merged label Sep 11, 2020

Borda approved these changes Sep 11, 2020

View reviewed changes

Borda merged commit 93cf6d0 into Lightning-AI:master Sep 11, 2020

SkafteNicki deleted the metrics/semi_supervised branch September 11, 2020 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metrics] class based embedding similarity + tests #3358

[Metrics] class based embedding similarity + tests #3358

SkafteNicki commented Sep 5, 2020

codecov bot commented Sep 6, 2020 •

edited

Loading

awaelchli left a comment

pep8speaks commented Sep 7, 2020 •

edited

Loading

Borda Sep 7, 2020

awaelchli Sep 7, 2020

Borda Sep 7, 2020

SkafteNicki Sep 8, 2020

Borda Sep 8, 2020

SkafteNicki Sep 11, 2020

[Metrics] class based embedding similarity + tests #3358

[Metrics] class based embedding similarity + tests #3358

Conversation

SkafteNicki commented Sep 5, 2020

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Sep 6, 2020 • edited Loading

Codecov Report

awaelchli left a comment

Choose a reason for hiding this comment

pep8speaks commented Sep 7, 2020 • edited Loading

Comment last updated at 2020-09-11 06:10:39 UTC

Borda Sep 7, 2020

Choose a reason for hiding this comment

awaelchli Sep 7, 2020

Choose a reason for hiding this comment

Borda Sep 7, 2020

Choose a reason for hiding this comment

SkafteNicki Sep 8, 2020

Choose a reason for hiding this comment

Borda Sep 8, 2020

Choose a reason for hiding this comment

SkafteNicki Sep 11, 2020

Choose a reason for hiding this comment

codecov bot commented Sep 6, 2020 •

edited

Loading

pep8speaks commented Sep 7, 2020 •

edited

Loading