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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate compute_on_step #789

Closed
SkafteNicki opened this issue Jan 20, 2022 · 7 comments 路 Fixed by #792
Closed

Deprecate compute_on_step #789

SkafteNicki opened this issue Jan 20, 2022 · 7 comments 路 Fixed by #792
Labels
API / design enhancement New feature or request refactoring refactoring and code health
Milestone

Comments

@SkafteNicki
Copy link
Member

馃殌 Feature

Deprecate compute_on_step argument in all modular metrics.

Motivation

Setting compute_on_step argument to False essentially turn forward into update, since this is the only code that is then being executed:
https://github.com/PyTorchLightning/metrics/blob/5b6cb1d0e735544e6c42cc39bbe7eb29561b75e1/torchmetrics/metric.py#L204-L205
It is sometimes a good thing to have multiple ways of doing certain operations, however in this case IMO it will be much more clear to the user that update always do not return anything and forward always returns something.

Pitch

The flag have been there since the beginning, but we as we are now starting the process of refactoring it is worth considering if it should still be part of the framework when it just adds a duplicate of a feature we already have.

Alternatives

Additional context

@SkafteNicki SkafteNicki added the enhancement New feature or request label Jan 20, 2022
@Borda
Copy link
Member

Borda commented Jan 20, 2022

I do not have any very strong opinion, I feel we can deprecate it... 馃惏
cc: @PyTorchLightning/core-metrics

@Borda Borda added this to the v0.8 milestone Jan 20, 2022
@Borda Borda added API / design refactoring refactoring and code health labels Jan 20, 2022
@ananthsub
Copy link
Contributor

@SkafteNicki - n00b question: why should forward call update vs the functional definition of the metric if available?

@SkafteNicki
Copy link
Member Author

@ananthsub forward will still have a double purpose of both returning the metric on the current batch and adding to the global metric state. So this will be the recommend way to do things in TM:

  • want the metric only on the current batch -> call functional
  • want to only aggregate the metric for all batches -> call update and then compute
  • want both metric on current batch + aggregation for all batches -> call forward and compute

@ananthsub
Copy link
Contributor

@SkafteNicki would this be the pseudocode for forward in the future?

def forward(*args, **args):
    self.update(*args, **args)
    return fn(*args, **args)

@Borda Borda closed this as completed in #792 Feb 8, 2022
@mmlynarik
Copy link

mmlynarik commented Feb 10, 2022

@ananthsub forward will still have a double purpose of both returning the metric on the current batch and adding to the global metric state. So this will be the recommend way to do things in TM:

* want the metric only on the current batch -> call functional

* want to only aggregate the metric for all batches -> call update and then compute

* want both metric on current batch + aggregation for all batches -> call forward and compute

Hi, can you please explain a little bit what is the difference between functional call and forward? I'm still not quite sure about all the details regarding metrics evaluation, e.g. I thought that self.train_acc(preds, y) is equal to self.train_acc.forward(preds, y) Thanks a lot!

@Borda Borda added this to the v0.8 milestone May 5, 2022
@ragavvenkatesan
Copy link
Contributor

+1 to this comment. Seems like,

  • forward updates the state and returns the current input's metric.
  • compute computes the metric from current state.
  • update only updates the current state.

Is this right?
Thanks.

@SkafteNicki
Copy link
Member Author

update only updates the current state.

Yes, that is completely right :]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API / design enhancement New feature or request refactoring refactoring and code health
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants