MetricTracker best_metric() returns the step instead of the best value

[bruce-willis]

🐛 Bug

MetricTracker.best_metric() function returns the step with the highest metric value instead of the best value itself.

To Reproduce

Please see colab for a full example and proposed solution.

Code sample

from torchmetrics import Accuracy, MetricTracker
import torch
_ = torch.manual_seed(42)
tracker = MetricTracker(Accuracy(num_classes=10))
for epoch in range(5):
    tracker.increment()
    for batch_idx in range(5):
        preds, target = torch.randint(10, (100,)), torch.randint(10, (100,))
        tracker.update(preds, target)
best_acc, which_epoch = tracker.best_metric(return_step=True)

best_acc2 = tracker.best_metric(return_step=False)

assert best_acc == best_acc2 # does not hold but should
assert which_epoch == best_acc2 # instead this condition holds but shouldn't

Expected behavior

Function best_metric without any parameters should return the best value:

Returns the highest metric out of all tracked.
best_metric function docstring

Environment

Does not depend on the version.

Additional context

Inconsistent behavior even with return_step=True

In fact, even with return_step=True the order is swapped, the function should return:

return idx.item(), best.item()

index first and best value second (source code), but in current version this function return best value first and index second (best_acc, which_epoch = ... from example).

Proposed solution

Use a namedtuple (values, indices) (pytorch max function doc) instead of implicit unpacking.

Replace the line with the error

idx[k], best[k] = out[0].item(), out[1].item()

from torchmetrics source code
with:

idx[k], best[k] = out.indices.item(), out.values.item()

It would be more readable and easier to understand and more prone to error.

If you agree with the proposal I can submit a follow-up pull request.

Proposed test case

Add the following test case:

# Assert that best_metric returns both index and value
val, idx = tracker.best_metric(return_step=True)
# Assert that best_metric returns just value without any parameters
val2 = tracker.best_metric()
assert val == val2

to the current test.

Backward compatibility

I think in order not to break backward compatibility this function should return the best value first and index second (as it shown in current examples). And probably the order should be placed in the documentation.

[github-actions]

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