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

Support for MetricTracker of MultioutputWrapper #1409

Closed
ValerianRey opened this issue Dec 24, 2022 · 4 comments · Fixed by #1608
Closed

Support for MetricTracker of MultioutputWrapper #1409

ValerianRey opened this issue Dec 24, 2022 · 4 comments · Fixed by #1608
Labels
enhancement New feature or request

Comments

@ValerianRey
Copy link
Contributor

ValerianRey commented Dec 24, 2022 •
edited
Loading

🚀 Feature

Currently, MetricTracker works in the following cases:

  • The base metric is of type Metric, and its compute method returns a single Tensor
  • The base metric is of type MetricCollection and its compute method returns a dict[str, Tensor]

I would like to add support for two other cases which I came across in my projects:

  • The base metric is of type MultioutputWrapper. Note that MultioutputWrapper is a subclass of Metric, but it is not supported by MetricTracker because its compute method returns a List[Tensor] rather than a single Tensor.
  • The base metric is a MetricCollection of MultioutputWrapper. In thise case the compute method returns a dict[str, List[Tensor]].

Motivation

  • I use MultioutputWrapper quite a lot for multi-regression problems. If I want to track its evolution over the epochs, I need to use a MetricTracker of MultioutputWrapper.
  • In some cases, I want to track multiple metrics (like MSE, MAE) for each of the outputs of my models, so I need to use a MetricTracker of MetricCollection of MultioutputWrapper.

Pitch

After looking at the code, here is my proposed solution:

  • Change MetricTracker's compute_all method to support both of the new cases mentioned above.
  • Change MetricTrackers best_metric method to support both of the new cases mentioned above.

Alternatives

Add a boolean parameter to MetricTracker to make it return a Tensor of higher dimension rather than a List[Tensor].

I don't really know which solution is best so I'll try to implement both in 2 different branches.
@ValerianRey ValerianRey added the enhancement New feature or request label Dec 24, 2022
@github-actions
Copy link

Hi! thanks for your contribution!, great first issue!

@ValerianRey
Copy link
Contributor Author

After looking at the code and starting to implement a solution, I completely updated the issue's description to make it much more clear.

ValerianRey pushed a commit to ValerianRey/metrics that referenced this issue Jan 2, 2023
Add test configs for MetricTracker (Lightning-AI#1409)
b669445 
* Add config corresponding to a `MetricTracker` of `MultioutputWrapper` of `MeanSquaredError`
* Add config corresponding to a `MetricTracker` of `MetricCollection` of (`MultioutputWrapper` of `MeanSquaredError` and `MultioutpuWrapper` of `MeanAbsoluteError`
* Change test checks so that they work when `tracker.compute()` returns tensors of dimension > 0
* Change checks on `MetricTracker.best_metric` to work when this method returns lists or dicts of lists
ValerianRey pushed a commit to ValerianRey/metrics that referenced this issue Jan 2, 2023
Add parameter stack_outputs to MultioutputWrapper (Lightning-AI#1409)
c1b180a 
* When this parameter is set to True, the output of the `compute_method` is stacked as a higher dimensional Tensor rather than a list
ValerianRey pushed a commit to ValerianRey/metrics that referenced this issue Jan 2, 2023
Change MetricTracker's best_metric (Lightning-AI#1409)
d32b66c 
* Add support to `MultioutputWrapper`
* Add support to `MetricCollection` of `MultioutputWrapper`
ValerianRey pushed a commit to ValerianRey/metrics that referenced this issue Jan 2, 2023
Update changelog (Lightning-AI#1409)
8df7d72 
* Add entry about Lightning-AI#1409 in the `Added` section
ValerianRey pushed a commit to ValerianRey/metrics that referenced this issue Jan 2, 2023
Add parameter stack_outputs to MultioutputWrapper (Lightning-AI#1409)
0487b40 
* When this parameter is set to True, the output of the `compute_method` is stacked as a higher dimensional Tensor rather than a list
ValerianRey pushed a commit to ValerianRey/metrics that referenced this issue Jan 2, 2023
Change MetricTracker's best_metric (Lightning-AI#1409)
ed447f8 
* Add support to `MultioutputWrapper`
* Add support to `MetricCollection` of `MultioutputWrapper`
ValerianRey pushed a commit to ValerianRey/metrics that referenced this issue Jan 2, 2023
Update changelog (Lightning-AI#1409)
2c45eae 
* Add entry about Lightning-AI#1409 in the `Added` section
ValerianRey added a commit to ValerianRey/metrics that referenced this issue Jan 5, 2023
@ValerianRey
Update test_tracker (Lightning-AI#1409)
4f18b4e 
* Add test configuration for test_tracker with a `MetricTracker` of `MultioutputWrapper`
* Change the test to support the case where the values returned by `MetricTracker` are lists
ValerianRey added a commit to ValerianRey/metrics that referenced this issue Jan 5, 2023
@ValerianRey
Update MetricTracker (Lightning-AI#1409)
4723316 
* Add support for `MultioutputMetric` as base metric in `compute_all`
* Add support for `MultioutputMetric` as base metric in `best_metric`
ValerianRey added a commit to ValerianRey/metrics that referenced this issue Jan 5, 2023
@ValerianRey
Update test_tracker (Lightning-AI#1409)
8c48585 
* Add test configuration for test_tracker with a `MetricTracker` of `MetricCollection` of `MultioutputWrapper`
* Change the test to support the case where the values returned by `MetricTracker` are dicts of lists
ValerianRey added a commit to ValerianRey/metrics that referenced this issue Jan 5, 2023
@ValerianRey
Update MetricTracker (Lightning-AI#1409)
50574b7 
* Add support for `MetricCollection` of `MultioutputMetric` as base metric in `compute_all`
* Add support for `MetricCollection` of `MultioutputMetric` as base metric in `best_metric`
ValerianRey added a commit to ValerianRey/metrics that referenced this issue Jan 5, 2023
@ValerianRey
Update changelog (Lightning-AI#1409)
df4dd05 
* Add entry about Lightning-AI#1409 in the `Added` section
@ValerianRey ValerianRey mentioned this issue Jan 6, 2023
Closed
@SkafteNicki
Copy link
Member

Hi @ValerianRey,
Just wanted to check in how much process you had on this issue? it seems you commit a lot of code that references this issue. Is it close to done or do you want me to take a stab at it :)

@ValerianRey
Copy link
Contributor Author

I added some new tests and the solution that makes them work when using a MetricTracker of MultioutputWrapper or a MetricTracker of MetricCollection of MultioutputWrapper.

Everything works, but I don't like it this way. The MetricTracker class becomes too complex for what it does. The return type of best_metric becomes even more complex for example: 12 possible return types.

I think this class needs some deep changes before we invest too much development into it (some changes that would not make it backward-compatible).
For now, we could just specify in the documentation which use-cases of MetricTracker work.

@SkafteNicki SkafteNicki mentioned this issue Mar 10, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Specify usage of metric tracker Lightning-AI/torchmetrics
2 participants
@SkafteNicki @ValerianRey