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.

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

[Metrics] MetricCollection #4318

Merged

Conversation

SkafteNicki
Copy link
Member

What does this PR do?

Adds generic MetricCollection object to chain together multiple Metric objects.
I seen both on slack and here on github (#4255) that users are defining dictionaries of metrics, to easy evaluate multiple at once. This PR adds support for this by introducing the MetricCollection object that can wrap multiple metrics into a single callable metric.
Also adds a prefix argument to self.log_dict such that users easily can reuse the same MetricCollection in both train, val, test.

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

  • Is this pull request ready for review? (if not, please submit in draft mode)

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 Outdated Show resolved Hide resolved
@Borda Borda requested a review from s-rog January 6, 2021 20:56
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.

LGTM, just I think that have seen some other metric collection from @tchaton?

CHANGELOG.md Outdated Show resolved Hide resolved
docs/source/metrics.rst Outdated Show resolved Hide resolved
docs/source/metrics.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@s-rog s-rog left a comment

Choose a reason for hiding this comment

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

Jury-rigged a similar class for my own project, glad to see a formalized API!

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Great job ! Small ideas to consider.

docs/source/metrics.rst Show resolved Hide resolved
pytorch_lightning/metrics/metric.py Show resolved Hide resolved
@SkafteNicki SkafteNicki merged commit 06668c0 into Lightning-AI:release/1.2-dev Jan 8, 2021
@SkafteNicki SkafteNicki deleted the metrics/metric_collection branch January 8, 2021 10:09
@chris-clem
Copy link

chris-clem commented Apr 19, 2021

Hi,

it seems that the prefix argument to self.log_dict was not added?

Edit:
I went through the commits and it was removed in this commit: 6986376. Was there a reason why it was deleted? It is still in the docs and seems like a useful argument.

@SkafteNicki
Copy link
Member Author

Hi @chris-clem,
The prefix arg was removed from the self.log_dict and instead added directly to the MetricCollection. Please see relevant section in the torchmetrics documentation https://torchmetrics.readthedocs.io/en/latest/pages/overview.html#metriccollection

@xfffrank
Copy link

Hi @chris-clem,
The prefix arg was removed from the self.log_dict and instead added directly to the MetricCollection. Please see relevant section in the torchmetrics documentation https://torchmetrics.readthedocs.io/en/latest/pages/overview.html#metriccollection

Hi @SkafteNicki,
Thank you! I just got the issue. This metric page is now outdated as the "prefix" argument has gone.

@SkafteNicki
Copy link
Member Author

@xfffrank I see your point. We are going to release v1.3 of lightning soon and then stable will instead take you to this page https://pytorch-lightning.readthedocs.io/en/latest/extensions/metrics.html which will redirect you to the torchmetrics documentation.

@xfffrank
Copy link

But I need to manually install torchmetrics 0.3.0 to have the "prefix" argument.

@SkafteNicki
Copy link
Member Author

But I need to manually install torchmetrics 0.3.0 to have the "prefix" argument.

Yes, torchmetrics 0.3.0 is in rc right now, which can be installed as:
pip install torchmetrics==0.3.0rc1
(0.3 is going to be out anytime soon, then you can just upgrade the package)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants