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 ComputationalMetric.forward() to not only consider the last batch #645

Merged
merged 17 commits into from
Nov 30, 2021

Conversation

Callidior
Copy link
Contributor

@Callidior Callidior commented Nov 29, 2021

What does this PR do?

Fixes #644

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.

tests/bases/test_composition.py Outdated Show resolved Hide resolved
tests/bases/test_composition.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #645 (5c26c93) into master (af461cc) will increase coverage by 0%.
The diff coverage is 77%.

@@          Coverage Diff          @@
##           master   #645   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files         161    161           
  Lines        5781   5794   +13     
=====================================
+ Hits         5501   5515   +14     
+ Misses        280    279    -1     

Callidior and others added 2 commits November 30, 2021 13:53
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
@Callidior
Copy link
Contributor Author

@SkafteNicki Thanks for your suggestions! I committed them as proposed.

@SkafteNicki
Copy link
Member

Hi @Callidior,
Just add changelog and improved one of test to get a bit better coverage :]

@mergify mergify bot added the ready label Nov 30, 2021
torchmetrics/metric.py Outdated Show resolved Hide resolved
torchmetrics/metric.py Outdated Show resolved Hide resolved
torchmetrics/metric.py Outdated Show resolved Hide resolved
@Borda Borda enabled auto-merge (squash) November 30, 2021 18:35
@Borda Borda added this to the v0.6 milestone Nov 30, 2021
@Borda Borda merged commit b4ec3c5 into Lightning-AI:master Nov 30, 2021
Borda pushed a commit that referenced this pull request Dec 5, 2021
…#645)

* Apply suggestions from code review

Co-authored-by: Björn Barz <bjoern.barz@zeiss.com>
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
(cherry picked from commit b4ec3c5)
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.

CompositionalMetric only returns value for last batch
3 participants