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

Add MetricInputTransformer #2392

Merged
merged 31 commits into from May 17, 2024
Merged

Add MetricInputTransformer #2392

merged 31 commits into from May 17, 2024

Conversation

lgienapp
Copy link
Contributor

@lgienapp lgienapp commented Feb 19, 2024

What does this PR do?

Fixes #2371

This PR adds a new kind of metric wrapper for data transformations. Transformations allow for modifications to the input a metric receives by wrapping its pred and target arguments. Transformations can be implemented by either subclassing the MetricInputTransformer base class and overriding the .transform_pred() and/or .transform_target() functions, or by supplying a lambda function via the LambdaInputTransformer. A BinaryTargetTransformer which casts target labels to 0/1 given a threshold is provided for convenience.

Before submitting
  • Was this discussed/agreed 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.


📚 Documentation preview 📚: https://torchmetrics--2392.org.readthedocs.build/en/2392/

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 19, 2024
@Borda Borda changed the title Add MetricInputTransformer, Fixes #2371 Add MetricInputTransformer Feb 19, 2024
lgienapp and others added 4 commits February 19, 2024 13:55
* origin/master:
  [pre-commit.ci] auto fixes from pre-commit.com hooks

# Conflicts:
#	src/torchmetrics/wrappers/transformations.py
#	tests/unittests/wrappers/test_transformations.py
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: Patch coverage is 38.77551% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 35%. Comparing base (4c2a143) to head (9c90ade).

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2392     +/-   ##
========================================
- Coverage      69%     35%    -35%     
========================================
  Files         313     314      +1     
  Lines       17615   17664     +49     
========================================
- Hits        12161    6096   -6065     
- Misses       5454   11568   +6114     

@lgienapp
Copy link
Contributor Author

lgienapp commented Feb 19, 2024

Unsure what to do about the failing mypy test - assigning the lambdas to overwrite the original class methods seems to be the most straightforward solution to supplying user-defined behaviour here; I would add the file to [[tool.mypy.overrides]] to mute the error (since its the only one for transformations.py) unless someone sees a better in-code solution?

@Borda Borda added this to the v1.4.0 milestone Feb 19, 2024
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

overall looks good to me :)

Copy link
Contributor

@baskrahmer baskrahmer left a comment

Choose a reason for hiding this comment

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

Looks good :)

tests/unittests/wrappers/test_transformations.py Outdated Show resolved Hide resolved
tests/unittests/wrappers/test_transformations.py Outdated Show resolved Hide resolved
lgienapp and others added 2 commits May 13, 2024 11:48
Co-authored-by: Bas Krahmer <baskrahmer@gmail.com>
Co-authored-by: Bas Krahmer <baskrahmer@gmail.com>
@mergify mergify bot added the ready label May 14, 2024
@mergify mergify bot removed the ready label May 15, 2024
@Borda
Copy link
Member

Borda commented May 15, 2024

@lgienapp I just fixed some typing, but suddenly, all tests are failing, Do you mind checking what is wrong... 🦩

@lgienapp
Copy link
Contributor Author

lgienapp commented May 15, 2024

Some unit tests check for error messages via regexes, and these had to be adapted to the typo fixes as well. Should all pass now.

@mergify mergify bot added the ready label May 15, 2024
@Borda Borda enabled auto-merge (squash) May 17, 2024 08:32
@Borda Borda disabled auto-merge May 17, 2024 10:37
Borda and others added 2 commits May 17, 2024 12:37
Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>
Copy link
Contributor

@stancld stancld left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for the addition!

@Borda Borda enabled auto-merge (squash) May 17, 2024 11:31
@Borda Borda merged commit d2f7c1f into Lightning-AI:master May 17, 2024
60 of 61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MetricWrapper for Target Binarization
5 participants