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

[ENH] -- CrossEncoder.rank #2947

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

it176131
Copy link
Contributor

@it176131 it176131 commented Sep 20, 2024

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.

	- 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.
@tomaarsen
Copy link
Collaborator

Hello!

I'm a bit hesitant to (already) add code with the |= operator, because Sentence Transformers is currently still supported on Python 3.8, whereas that operator is not.
However, in about 1 month, Python 3.8 will become deprecated, and then I'll have no issues merging this PR.

  • Tom Aarsen

@it176131
Copy link
Contributor Author

Hello!

I'm a bit hesitant to (already) add code with the |= operator, because Sentence Transformers is currently still supported on Python 3.8, whereas that operator is not. However, in about 1 month, Python 3.8 will become deprecated, and then I'll have no issues merging this PR.

  • Tom Aarsen

Good catch! Interesting that the 3.8 tests didn't raise an exception 🤔

@tomaarsen
Copy link
Collaborator

Huh, you're right. Perhaps rank isn't under test, or I'm wrong and |= also works on Python 3.8.

@it176131
Copy link
Contributor Author

Huh, you're right. Perhaps rank isn't under test, or I'm wrong and |= also works on Python 3.8.

I'm not seeing a check in the current test, but I'd be willing to add one for better coverage.

@tomaarsen
Copy link
Collaborator

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.
@it176131 it176131 force-pushed the CrossEncoder.run-enhancement branch from f1b939d to b559628 Compare October 1, 2024 15:23
@it176131
Copy link
Contributor Author

it176131 commented Oct 1, 2024

That would be much appreciated!

Updated the test!

@it176131
Copy link
Contributor Author

it176131 commented Oct 8, 2024

@tomaarsen is there anything else we need before merging?

@tomaarsen
Copy link
Collaborator

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 plan to include this in the next release, which should be very soon (i.e. this week, I imagine).

  • Tom Aarsen

@tomaarsen tomaarsen merged commit 07ae865 into UKPLab:master Oct 8, 2024
11 checks passed
@it176131 it176131 deleted the CrossEncoder.run-enhancement branch October 8, 2024 14:33
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.

2 participants