Added add_metrics method to MetricCollection #221

IgorHoholko · 2021-05-02T08:56:42Z

Fixes #203

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?

What does this PR do?

add_metrics() method for MetricCollection class.

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 🙃

codecov · 2021-05-02T09:22:03Z

Codecov Report

Merging #221 (cd1db7a) into master (cb6899b) will decrease coverage by 8.80%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   96.78%   87.97%   -8.81%     
==========================================
  Files         184      184              
  Lines        5976     5965      -11     
==========================================
- Hits         5784     5248     -536     
- Misses        192      717     +525

Flag	Coverage Δ
Linux	`79.12% <84.61%> (+0.01%)`	⬆️
Windows	`79.12% <84.61%> (+0.01%)`	⬆️
cpu	`79.12% <84.61%> (-17.67%)`	⬇️
gpu	`96.78% <ø> (+<0.01%)`	⬆️
macOS	`79.12% <84.61%> (-17.67%)`	⬇️
pytest	`87.97% <84.61%> (-8.81%)`	⬇️
python3.6	`?`
python3.8	`?`
python3.9	`?`
torch1.3.1	`?`
torch1.4.0	`?`
torch1.8.1	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
torchmetrics/collections.py	`74.71% <84.61%> (-24.12%)`	⬇️
torchmetrics/utilities/distributed.py	`17.24% <0.00%> (-81.04%)`	⬇️
torchmetrics/classification/auc.py	`47.82% <0.00%> (-52.18%)`	⬇️
torchmetrics/functional/classification/auroc.py	`46.15% <0.00%> (-40.01%)`	⬇️
torchmetrics/metric.py	`55.98% <0.00%> (-39.52%)`	⬇️
torchmetrics/functional/regression/psnr.py	`60.60% <0.00%> (-36.37%)`	⬇️
torchmetrics/functional/classification/accuracy.py	`57.57% <0.00%> (-36.37%)`	⬇️
...chmetrics/functional/classification/stat_scores.py	`66.66% <0.00%> (-33.34%)`	⬇️
...rics/functional/classification/confusion_matrix.py	`66.66% <0.00%> (-33.34%)`	⬇️
torchmetrics/utilities/checks.py	`60.54% <0.00%> (-31.90%)`	⬇️
... and 231 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb6899b...cd1db7a. Read the comment docs.

SkafteNicki

LGTM

Borda · 2021-05-03T17:21:38Z

torchmetrics/collections.py

@@ -185,6 +147,51 @@ def persistent(self, mode: bool = True) -> None:
        for _, m in self.items():
            m.persistent(mode)

+    def add_metrics(self, metrics: Union[Metric, Sequence[Metric], Dict[str, Metric]],


Suggested change

def add_metrics(self, metrics: Union[Metric, Sequence[Metric], Dict[str, Metric]],

def append(self, metrics: Union[Metric, Sequence[Metric], Dict[str, Metric]],

would it be a better name?

I think it's fine to keep the name different. If we say append it might imply that it won't work as extend while here it does.

add_metrics seems more appropriate for a Collection.

maximsch2 · 2021-05-03T17:23:38Z

torchmetrics/collections.py

+        if isinstance(metrics, Metric):
+            # set compatible with original type expectations
+            metrics = [metrics]
+        if isinstance(metrics, Sequence):


I understand this is just copying what we had here before, but we should probably have Iterable here instead of Sequence as we doin't really care about indexing.

but iterable does not have len, right?

We are not using len though, we are just directly converting to a list in the next line.

maximsch2 · 2021-05-03T17:25:34Z

torchmetrics/collections.py

@@ -185,6 +147,51 @@ def persistent(self, mode: bool = True) -> None:
        for _, m in self.items():
            m.persistent(mode)

+    def add_metrics(self, metrics: Union[Metric, Sequence[Metric], Dict[str, Metric]],


It should be Mapping[str, Metrics], not Dict + similar a check below.

maximsch2 · 2021-05-03T17:26:12Z

torchmetrics/collections.py

@@ -185,6 +147,51 @@ def persistent(self, mode: bool = True) -> None:
        for _, m in self.items():
            m.persistent(mode)

+    def add_metrics(self, metrics: Union[Metric, Sequence[Metric], Dict[str, Metric]],


I think it's fine to keep the name different. If we say append it might imply that it won't work as extend while here it does.

Borda · 2021-05-03T19:34:12Z

@IgorHoholko mind check the comments above?

tchaton

LGTM ! Worth to increase the coverage slightly.

tchaton · 2021-05-04T07:45:49Z

torchmetrics/collections.py

@@ -185,6 +147,51 @@ def persistent(self, mode: bool = True) -> None:
        for _, m in self.items():
            m.persistent(mode)

+    def add_metrics(self, metrics: Union[Metric, Sequence[Metric], Dict[str, Metric]],


add_metrics seems more appropriate for a Collection.

added add_metrics method to MetricCollection

cc91ef3

IgorHoholko changed the title ~~added add_metrics method to MetricCollection (#203)~~ Added add_metrics method to MetricCollection (#203) May 2, 2021

SkafteNicki added the enhancement New feature or request label May 3, 2021

SkafteNicki added this to the v0.4 milestone May 3, 2021

changelog

fb8e815

SkafteNicki approved these changes May 3, 2021

View reviewed changes

SkafteNicki marked this pull request as ready for review May 3, 2021 07:25

SkafteNicki requested review from ananyahjha93, Borda, justusschock and tchaton as code owners May 3, 2021 07:25

SkafteNicki added the ready label May 3, 2021

Merge branch 'master' into metricsCollection_addMetrics

115aec5

Borda changed the title ~~Added add_metrics method to MetricCollection (#203)~~ Added add_metrics method to MetricCollection May 3, 2021

Borda enabled auto-merge (squash) May 3, 2021 17:20

Borda reviewed May 3, 2021

View reviewed changes

mergify bot added 2 commits May 3, 2021 17:23

Merge branch 'master' into metricsCollection_addMetrics

55d2896

Merge branch 'master' into metricsCollection_addMetrics

5a3094f

maximsch2 reviewed May 3, 2021

View reviewed changes

Merge branch 'master' into metricsCollection_addMetrics

c6037da

mergify bot added the has conflicts label May 3, 2021

Merge branch 'master' into metricsCollection_addMetrics

1263786

mergify bot removed the has conflicts label May 4, 2021

tchaton approved these changes May 4, 2021

View reviewed changes

mergify bot added 2 commits May 4, 2021 10:01

Merge branch 'master' into metricsCollection_addMetrics

3b2fdf9

Merge branch 'master' into metricsCollection_addMetrics

cd1db7a

Borda merged commit 27bc28e into Lightning-AI:master May 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added add_metrics method to MetricCollection #221

Added add_metrics method to MetricCollection #221

IgorHoholko commented May 2, 2021 •

edited by SkafteNicki

Loading

codecov bot commented May 2, 2021 •

edited

Loading

SkafteNicki left a comment

Borda May 3, 2021

maximsch2 May 3, 2021

tchaton May 4, 2021

maximsch2 May 3, 2021

Borda May 3, 2021

maximsch2 May 3, 2021

maximsch2 May 3, 2021

maximsch2 May 3, 2021

Borda commented May 3, 2021

tchaton left a comment •

edited

Loading

tchaton May 4, 2021

	def add_metrics(self, metrics: Union[Metric, Sequence[Metric], Dict[str, Metric]],
	def append(self, metrics: Union[Metric, Sequence[Metric], Dict[str, Metric]],

Added add_metrics method to MetricCollection #221

Added add_metrics method to MetricCollection #221

Conversation

IgorHoholko commented May 2, 2021 • edited by SkafteNicki Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

codecov bot commented May 2, 2021 • edited Loading

Codecov Report

SkafteNicki left a 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

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

Borda commented May 3, 2021

tchaton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorHoholko commented May 2, 2021 •

edited by SkafteNicki

Loading

codecov bot commented May 2, 2021 •

edited

Loading

tchaton left a comment •

edited

Loading