Deprecate/compute on step

[SkafteNicki]

What does this PR do?

Fixes #789
Deprecates compute_on_step argument.

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?

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]

Codecov Report

Merging #792 (71176b1) into master (c0e4250) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #792   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         166    166           
  Lines        6795   6796    +1     
=====================================
- Hits         6469   6460    -9     
- Misses        326    336   +10     

[Borda]

@SkafteNicki mind checking the last two failing tests?

[Borda]

FAILED tests/bases/test_composition.py::test_compositional_metrics_forward[4-False]
FAILED tests/bases/test_composition.py::test_compositional_metrics_forward[metric_b1-False]

[Alec-Stashevsky]

@SkafteNicki why was compute_on_step deprecated? I use torchmetrics with base PyTorch (non Lightning) and found that having compute_on_step == True (the default) drastically affected my results. I needed to keep it off. How would I go about that in version 0.8.0?

Thanks

[fierval]

Agree with @Alec-Stashevsky 's comment. If I am reading the code correctly:

    def forward(self, *args: Any, **kwargs: Any) -> Any:
        """Automatically calls ``update()``.

        Returns the metric value over inputs if ``compute_on_step`` is True.
        """
        # add current step
        if self._is_synced:
            raise TorchMetricsUserError(
                "The Metric shouldn't be synced when performing ``update``. "
                "HINT: Did you forget to call ``unsync`` ?."
            )

        # global accumulation
        self.update(*args, **kwargs)

        if self.compute_on_step:
            self._to_sync = self.dist_sync_on_step
            # skip restore cache operation from compute as cache is stored below.
            self._should_unsync = False

            # save context before switch
            cache = {attr: getattr(self, attr) for attr in self._defaults}

            # call reset, update, compute, on single batch
            self._enable_grad = True  # allow grads for batch computation
            self.reset()
            self.update(*args, **kwargs)
            self._forward_cache = self.compute()

            # restore context
            for attr, val in cache.items():
                setattr(self, attr, val)
            self._is_synced = False

            self._should_unsync = True
            self._to_sync = True
            self._computed = None
            self._enable_grad = False

            return self._forward_cache

This used to be the forward function in 0.7.3. Now that the if statement is removed, does it mean that the metric is finalized & reset on every batch? That's not what I really want.

[SkafteNicki]

Hi @Alec-Stashevsky
If you set compute_on_step=False you can directly see in the code that @fierval posted that calling forward is the exact same as calling update directly. Therefore instead of using a metric like this:

metric = Metric(compute_on_step=False)
metric(input)

you should instead just do

metric = Metric()
metric.update(input)

@fierval the change in 0.8 just have the effect that forward will always accumulate to the global metrics state (the first call to update) AND also calculate the metric on the current batch. The metric is therefore only finalized on the current batch and not the global state.