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

Metric retrieval recall at precision #951

Merged

Conversation

MrShevan
Copy link
Contributor

@MrShevan MrShevan commented Apr 12, 2022

What does this PR do?

Hello, @stancld ! It's a draft PR for issue: Fixes #780

I still think about how to add more tests, testing pattern in this repo is pretty hard, but the general idea of RetrievalRecallAtFixedPrecision is implemented.

Keep working 🛠

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 🙃

@Borda
Copy link
Member

Borda commented Apr 16, 2022

@MrShevan, how is it going here? 🐰

@MrShevan
Copy link
Contributor Author

Hi! Sorry was busy at the end of the week, I keep working on PR. I'm still thinking about my implementation. Was it right to inherit from Metric class instead RetrievalMetric? I decided to do so to compute recall and precision metrics for all k and return two values: best recall and the value in which it is achieved.

@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #951 (f9dd87f) into master (f8ff34e) will decrease coverage by 0%.
The diff coverage is 86%.

@@          Coverage Diff           @@
##           master   #951    +/-   ##
======================================
- Coverage      95%    95%    -0%     
======================================
  Files         177    179     +2     
  Lines        7522   7631   +109     
======================================
+ Hits         7148   7229    +81     
- Misses        374    402    +28     

@enuk1dze enuk1dze force-pushed the metric_retrieval_recall_at_precision branch from 09a8cad to e0ea387 Compare April 18, 2022 18:36
@MrShevan
Copy link
Contributor Author

@stancld @Borda @SkafteNicki
Thanks for your comments :) I tried to take into account all your comments:

  1. Firstly I I concentrated on the precision-recall curve as noticed @stancld instead of the varying k and made it main logic as functional, which used in module, thanks @Borda for advise.
  2. I resolved all comments from @SkafteNicki.
  3. I spent a little more time for such tests, but now they are all passed, when running locally. :)

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.

LGTM

@mergify mergify bot added the ready label Apr 29, 2022
@Borda Borda merged commit 1bc6c47 into Lightning-AI:master Apr 29, 2022
@MrShevan
Copy link
Contributor Author

@Borda @stancld @SkafteNicki
I enjoyed working on this issue, thanks a lot for your involvement :]

@Borda
Copy link
Member

Borda commented Apr 29, 2022

I enjoyed working on this issue, thanks a lot for your involvement :]

great contribution!

@stancld
Copy link
Contributor

stancld commented Apr 29, 2022

@MrShevan Thanks a lot for your great contribution! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RetrievalRecallAtFixedPrecision
6 participants