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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally Avoid recomputing features #722

Merged
merged 17 commits into from
Apr 11, 2022
Merged

Conversation

justusschock
Copy link
Member

What does this PR do?

Optionally avoids recomputing the features.

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 🙃

@Borda Borda added this to the v0.8 milestone Jan 6, 2022
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclude_states should also be added to:

  • MetricCollection
  • MinMaxMetric
  • MultioutputWrapper
  • MetricTracker

for consistency

torchmetrics/metric.py Outdated Show resolved Hide resolved
torchmetrics/image/fid.py Outdated Show resolved Hide resolved
torchmetrics/image/kid.py Outdated Show resolved Hide resolved
Borda and others added 2 commits January 20, 2022 17:28
@Borda
Copy link
Member

Borda commented Jan 20, 2022

@justusschock still draft or ready to read? 🐰

@Borda
Copy link
Member

Borda commented Jan 20, 2022

is it related to #709? 🐰

@justusschock
Copy link
Member Author

@Borda waiting for some user feedback on slack :)

I think not directly related to #709

@Borda Borda changed the title Optionally Avoid recomputing features [blocked by 709] Optionally Avoid recomputing features Feb 5, 2022
@Borda Borda changed the title [blocked by 709] Optionally Avoid recomputing features Optionally Avoid recomputing features Feb 8, 2022
@Borda
Copy link
Member

Borda commented Feb 8, 2022

waiting for some user feedback on slack :)

what kind of feedback, could you please remind me of the thread... 🐰

@ebartrum
Copy link

Copy link
Member Author

@justusschock justusschock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid bug when passing None

torchmetrics/image/fid.py Outdated Show resolved Hide resolved
@SkafteNicki
Copy link
Member

@justusschock instead of making this PR have influence on the base class, what about re-implementing just reset in the relevant metrics:

def reset(self):
  if not self.reset_real_features:
    # remove temporarily to avoid resetting
    value = self._defaults.pop('real_features')
    super().reset()
    self._defaults['real_features'] = value
  else:
    super().reset()

this feature seems to only be relevant to Inception, FID and KID

@justusschock
Copy link
Member Author

@SkafteNicki works for me. However, afaik there are some similar metrics in NLP which might be added some day. This is why I aimed for a more general approach here. I'm fine with both, just let me know your preference :)

@SkafteNicki
Copy link
Member

@justusschock I would go with the more local solution for the affected metrics for now. If we then in the future have other metrics that need a similar feature then I am fine with revisiting this :]

@Borda Borda modified the milestones: v0.8, v0.9 Mar 22, 2022
@SkafteNicki SkafteNicki added the enhancement New feature or request label Apr 11, 2022
@SkafteNicki SkafteNicki modified the milestones: v0.9, v0.8 Apr 11, 2022
@SkafteNicki SkafteNicki marked this pull request as ready for review April 11, 2022 08:16
@mergify mergify bot requested a review from a team April 11, 2022 08:17
@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #722 (e817ad1) into master (0d56376) will decrease coverage by 12%.
The diff coverage is 91%.

@@          Coverage Diff           @@
##           master   #722    +/-   ##
======================================
- Coverage      95%    83%   -12%     
======================================
  Files         173    173            
  Lines        7381   7401    +20     
======================================
- Hits         7012   6149   -863     
- Misses        369   1252   +883     

@SkafteNicki
Copy link
Member

@justusschock took the liberty to update the PR, please take a look :]

@mergify mergify bot added the ready label Apr 11, 2022
@justusschock
Copy link
Member Author

@SkafteNicki Thanks for updating. Seems you were faster :) LGTM :)

@justusschock justusschock merged commit 97f2bf5 into master Apr 11, 2022
@justusschock justusschock deleted the fix/not_recompute_features branch April 11, 2022 08:53
@Borda Borda added this to the v0.8 milestone May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants