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 rerank recall metric to unitxt #662

Merged
merged 10 commits into from
Apr 2, 2024
Merged

Add rerank recall metric to unitxt #662

merged 10 commits into from
Apr 2, 2024

Conversation

jlqibm
Copy link
Contributor

@jlqibm jlqibm commented Mar 13, 2024

Closes #661 in support of adding perplexity reranking to fm-eval.

@jlqibm
Copy link
Contributor Author

jlqibm commented Mar 13, 2024

@yoavkatz please review

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

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

Project coverage is 90.87%. Comparing base (4274d2b) to head (a6c64c7).

Files Patch % Lines
src/unitxt/metrics.py 22.50% 31 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #662      +/-   ##
==========================================
- Coverage   91.13%   90.87%   -0.27%     
==========================================
  Files          98       98              
  Lines        9897     9937      +40     
==========================================
+ Hits         9020     9030      +10     
- Misses        877      907      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

requirements/base.rqr Outdated Show resolved Hide resolved
Copy link
Member

@elronbandel elronbandel left a comment

Choose a reason for hiding this comment

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

Fix the requirements according to unitxt no-requirements policy explained in the comments

@elronbandel
Copy link
Member

You need to add the pytrec_eval to therequirements/tests.rqr

@jlqibm
Copy link
Contributor Author

jlqibm commented Mar 25, 2024

@elronbandel pytrec-eval is added.

@yoavkatz
Copy link
Member

The tests failed with this:

image

The pre-commit failed with this:

image

(Do you have pre-commit installed with pre-commit install ? )

@jlqibm
Copy link
Contributor Author

jlqibm commented Mar 27, 2024

@yoavkatz The pandas package is missing. So there are 2 question here. One, why didn't this fail at the import statement? Two, which requirements file does pandas need to be added to.
Sorry about the test failure.

I have precommit installed now.

src/unitxt/metrics.py Outdated Show resolved Hide resolved
@yoavkatz
Copy link
Member

@yoavkatz The pandas package is missing. So there are 2 question here. One, why didn't this fail at the import statement? Two, which requirements file does pandas need to be added to. Sorry about the test failure.

There is a missing import to pandas. In general, if metrics needs specific packages like pandas or trec_eval, we import inside the relevant metric code, and not at the top of the metrics.py file, so If people don't need the metric, they will not need the include.

@jlqibm jlqibm requested a review from elronbandel March 28, 2024 17:22
@jlqibm
Copy link
Contributor Author

jlqibm commented Mar 28, 2024

@elronbandel I already fixed the requested change but github still thinks it's pending.

@jlqibm
Copy link
Contributor Author

jlqibm commented Apr 2, 2024

@yoavkatz can you tell what's holding up this merge? There appears to be an outstanding change request but I already fixed that a while ago.

@yoavkatz
Copy link
Member

yoavkatz commented Apr 2, 2024

I reviewed the code, and it looks fine so I approved.

@elronbandel - I think we the main issue is that the coverage is low - although tests were added (but in the prepare file).
The reason is that coverage is not measured on the prepare files.

We should probably give guidance to write the test in test_metrics and not in the prepare file (unless using a wrapper like HuggingFaceMetric).

@yoavkatz yoavkatz enabled auto-merge (squash) April 2, 2024 20:29
@yoavkatz yoavkatz merged commit ea47b07 into IBM:main Apr 2, 2024
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add recall at rank metric
3 participants