-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[ENH] -- CrossEncoder.rank
#2947
[ENH] -- CrossEncoder.rank
#2947
Conversation
- Replaced `range(len(scores))` with `enumerate(scores)` and used index (`i`) and `score` directly.
- Removed need for `else` by appending to `results` once, then conditionally updating appended value given `return_documents`.
- Used parameter name to match other kwarg usage.
Hello! I'm a bit hesitant to (already) add code with the
|
Good catch! Interesting that the 3.8 tests didn't raise an exception 🤔 |
Huh, you're right. Perhaps |
I'm not seeing a check in the current test, but I'd be willing to add one for better coverage. |
That would be much appreciated! |
- Updated dictionary unioning to use Python 3.8 compatible syntax in :method:`CrossEncoder.rank`. modified: tests/test_cross_encoder.py - Parametrized test :callable:`test_rank` with checks for `return_documents` argument.
- Used a `FixtureRequest` to do further testing when `return_documents` is True.
- Changed method of unioning dictionary to `update` in :method:`CrossEncoder.rank` to avoid unpacking original dictionary.
f1b939d
to
b559628
Compare
Updated the test! |
@tomaarsen is there anything else we need before merging? |
I don't believe so! I think we're all set now, I hadn't merged it yet as I was wrapping up some work in another PR.
|
I found some spots in the :method:
CrossEncoder.rank
that could be updated to reflect best practices. Feel free to cherry-pick or reject.Tested with Python 3.8 for minimum version coverage.