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

Added adaptive_k argument to IR Precision metric #910

Merged
merged 10 commits into from
Mar 28, 2022
Merged

Added adaptive_k argument to IR Precision metric #910

merged 10 commits into from
Mar 28, 2022

Conversation

lucadiliello
Copy link
Contributor

What does this PR do?

Fixes #886

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 🙃

@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #910 (c9fb6e1) into master (d8c5e11) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #910   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         173    173           
  Lines        7339   7344    +5     
=====================================
- Hits         6971   6963    -8     
- Misses        368    381   +13     

@Borda Borda added the bug / fix Something isn't working label Mar 24, 2022
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.

pls add changelog entry

tests/retrieval/test_precision.py Outdated Show resolved Hide resolved
@SkafteNicki SkafteNicki added enhancement New feature or request and removed bug / fix Something isn't working labels Mar 24, 2022
@SkafteNicki SkafteNicki added this to the v0.8 milestone Mar 24, 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.

I assume that the parameter should also be added to the modular metrics

torchmetrics/retrieval/precision.py Outdated Show resolved Hide resolved
@Borda
Copy link
Member

Borda commented Mar 24, 2022

@lucadiliello pls check #910 (review) ^^

@lucadiliello
Copy link
Contributor Author

LGTM

@Borda
Copy link
Member

Borda commented Mar 25, 2022

I assume that the parameter should also be added to the modular metrics

@lucadiliello not sure if I understand, so does the parameter shall also be added to the modular metrics?

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.

LGTM

@lucadiliello
Copy link
Contributor Author

lucadiliello commented Mar 25, 2022

I assume that the parameter should also be added to the modular metrics

@lucadiliello not sure if I understand, so does the parameter shall also be added to the modular metrics?

I added it both to the functional and the modular version of RetrievalPrecision. I didn’t add it to the other retrieval metrics because it makes no sense.

@Borda Borda enabled auto-merge (squash) March 25, 2022 16:50
CHANGELOG.md Outdated Show resolved Hide resolved
@mergify mergify bot added the ready label Mar 25, 2022
@Borda Borda merged commit 25fdbae into Lightning-AI:master Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow automatically adjusting k for RetrievalPrecision when k > num(retrieved documents)
5 participants