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

Fix type hints #1472

Merged
merged 5 commits into from Feb 1, 2023
Merged

Fix type hints #1472

merged 5 commits into from Feb 1, 2023

Conversation

ValerianRey
Copy link
Contributor

What does this PR do?

This PR fixes some type hints in MetricTracker, which do not correspond to the possible return types of the methods.
It also fixes in a test a confusion in the order of the tuple (value, index) returned by MetricTracker.best_metric (the order now matches the type hint and the type hint now matches the doc).

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? => No need
  • Did you write any new necessary tests? => No need

PR review

Did you have fun?

Yes!

@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #1472 (12209f7) into master (2ce0efb) will decrease coverage by 52%.
The diff coverage is 100%.

❗ Current head 12209f7 differs from pull request most recent head 8949ee2. Consider uploading reports for the commit 8949ee2 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1472     +/-   ##
========================================
- Coverage      90%     38%    -52%     
========================================
  Files         213     213             
  Lines       10921   10921             
========================================
- Hits         9794    4162   -5632     
- Misses       1127    6759   +5632     

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.

Super cool :)

src/torchmetrics/wrappers/tracker.py Outdated Show resolved Hide resolved
src/torchmetrics/wrappers/tracker.py Show resolved Hide resolved
@mergify mergify bot added the ready label Feb 1, 2023
@SkafteNicki SkafteNicki merged commit fbdc393 into Lightning-AI:master Feb 1, 2023
Borda pushed a commit that referenced this pull request Feb 20, 2023
* Fix return type in MetricTracker.compute_all

* Fix return type in MetricTracker.best_metric

* Fix test_best_metric_for_not_well_defined_metric_collection

* Assign MetricTracker.best_metric return value to a tuple of the form (best, idx), instead of (idx, best).

* changelog

* add docstring

---------

Co-authored-by: SkafteNicki <skaftenicki@gmail.com>
(cherry picked from commit fbdc393)
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.

None yet

4 participants