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

fixes metric hashing #478

Merged
merged 11 commits into from
Aug 24, 2021
Merged

fixes metric hashing #478

merged 11 commits into from
Aug 24, 2021

Conversation

justusschock
Copy link
Member

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?

What does this PR do?

We need the hashing to be unique. PyTorch relies on that for children discoverability.

before hash(AUROC()) == hash(AUROC()) evaluated to true when metrics had an empty non-tensor state (like an empty list) causing some attributes to be not appearing in the children.
Fixes # (issue).

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 🙃

@justusschock justusschock added bug / fix Something isn't working Priority Critical task/issue Lightning labels Aug 24, 2021
@justusschock justusschock self-assigned this Aug 24, 2021
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGMT !

torchmetrics/metric.py Outdated Show resolved Hide resolved
Co-authored-by: thomas chaton <[email protected]>
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #478 (d52458f) into master (c98e659) will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #478   +/-   ##
=====================================
  Coverage      96%    96%           
=====================================
  Files         130    130           
  Lines        4301   4301           
=====================================
  Hits         4126   4126           
  Misses        175    175           

tests/helpers/testers.py Outdated Show resolved Hide resolved
tests/bases/test_hashing.py Outdated Show resolved Hide resolved
tests/bases/test_hashing.py Outdated Show resolved Hide resolved
@Borda Borda requested a review from SkafteNicki August 24, 2021 09:11
@mergify mergify bot added the ready label Aug 24, 2021
@SkafteNicki SkafteNicki merged commit 3257c60 into master Aug 24, 2021
@SkafteNicki SkafteNicki deleted the fix_unique_hash branch August 24, 2021 11:31
Borda pushed a commit that referenced this pull request Aug 27, 2021
* fixes metric hashing

* Update torchmetrics/metric.py

* Apply suggestions from code review

* chlog

* fix test

* fix decorator

* fix decorator

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Nicki Skafte <[email protected]>
Co-authored-by: Jirka <[email protected]>
(cherry picked from commit 3257c60)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working Lightning Priority Critical task/issue ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants