Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Base class for metrics #1293

Closed
wants to merge 10 commits into from
Closed

Add Base class for metrics #1293

wants to merge 10 commits into from

Conversation

justusschock
Copy link
Member

@justusschock justusschock commented Mar 30, 2020 •
edited by Borda
Loading

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 to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Implements a base class for new metrics.
Fixes #1294

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 🙃

A review of the general interface would be nice, before I start to implement the necessary tests

@justusschock justusschock added feature Is an improvement or enhancement help wanted Open to be worked on labels Mar 30, 2020
@justusschock justusschock requested review from Borda and a team March 30, 2020 12:47
@justusschock justusschock self-assigned this Mar 30, 2020
@Borda Borda changed the base branch from master to metrics March 30, 2020 15:23
@codecov
Copy link

codecov bot commented Mar 30, 2020

Codecov Report

Merging #1293 into metrics will decrease coverage by 1%.
The diff coverage is 38%.

@@           Coverage Diff           @@
##           metrics   #1293   +/-   ##
=======================================
- Coverage       92%     91%   -1%     
=======================================
  Files           62      63    +1     
  Lines         3188    3230   +42     
=======================================
+ Hits          2920    2936   +16     
- Misses         268     294   +26

@pep8speaks
Copy link

pep8speaks commented Mar 31, 2020 •
edited
Loading

Hello @justusschock! Thanks for updating this PR.

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

Comment last updated at 2020-03-31 13:33:44 UTC

@mergify mergify bot requested a review from a team March 31, 2020 07:22
@justusschock justusschock changed the title WIP: Add prototype for base metric Add Base class for metrics Mar 31, 2020
@justusschock justusschock marked this pull request as ready for review March 31, 2020 07:35
@justusschock justusschock requested a review from Borda March 31, 2020 07:35
Borda
Borda reviewed Mar 31, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

GREAT work, just a quick pass, need to read again... 🤖

pytorch_lightning/metrics/metric.py
class BaseMetric(torch.nn.Module, ABC):
def __init__(self, name: str,
reduce_group: Optional[Any] = torch.distributed.group.WORLD,
reduce_op: Optional[Any] = torch.distributed.ReduceOp.SUM):
Copy link
Member

Choose a reason for hiding this comment

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

inst mean more frequently used?

Copy link
Member Author

Choose a reason for hiding this comment

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

mean is not available as a reduction op :)
https://pytorch.org/docs/stable/distributed.html#torch.distributed.ReduceOp

pytorch_lightning/metrics/metric.py Outdated
metric value

"""
raise NotImplementedError
Copy link
Member

Choose a reason for hiding this comment

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

for abstract no need for this raise

Copy link
Member Author

@justusschock justusschock Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I know, but it doesn't matter if you put pass or raise NotImplementedError there and I found this to be more intuitive, but we can still change this, if you want...

Copy link
Member

Choose a reason for hiding this comment

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

this line won't be ever tested so lover test coverage count... lol

pytorch_lightning/metrics/metric.py
return elem_type([func(r) for r in result])
# not possible to reduce this type
else:
return result
Copy link
Member

Choose a reason for hiding this comment

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

this is not safe since all result assignment is under some conditions...
rather instead each return do result assignment and always return result

Copy link
Member Author

Choose a reason for hiding this comment

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

this should be safe, since we just return the input variable as is, if there is nothing to sync...

justusschock and others added 2 commits March 31, 2020 15:08
@justusschock @Borda
Apply suggestions from code review
5583844 
suggested changes

Co-Authored-By: Jirka Borovec <[email protected]>
@justusschock
Update metric.py
9f892a7
@justusschock justusschock mentioned this pull request Apr 1, 2020
Merged
5 tasks
@justusschock
Copy link
Member Author

closed in favour of #1326

@Borda
Copy link
Member

Borda commented Apr 1, 2020

@justusschock what os the difference?

@Borda Borda deleted the base_metric branch April 1, 2020 18:16
@justusschock
Copy link
Member Author

@Borda the new version is cleaner and has more capabilities. This includes the changes, I had to make when implementing the scikit part. It was easier to make it as a separate or than to incorporate it into this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda left review comments

@ethanwharris ethanwharris Awaiting requested review from ethanwharris

@jeremyjordan jeremyjordan Awaiting requested review from jeremyjordan

@neggert neggert Awaiting requested review from neggert

@tullie tullie Awaiting requested review from tullie

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees

@justusschock justusschock

Labels
feature Is an improvement or enhancement help wanted Open to be worked on
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metrics: Base Metric
3 participants
@justusschock @pep8speaks @Borda