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

Fix get_train_with_targets_for_reranker method #244

Open
wants to merge 18 commits into
base: experimental/two_stage
Choose a base branch
from

Conversation

olesyabulgakova
Copy link
Collaborator

We make changes to the get_train_with_targets_for_reranker method to separate the retrieval of sampled candidates and unsampled candidates from first-stage candidate generators for the reranker.

@olesyabulgakova olesyabulgakova self-assigned this Jan 15, 2025
rectools/models/ranking/candidate_ranking.py Outdated Show resolved Hide resolved
rectools/models/ranking/candidate_ranking.py Outdated Show resolved Hide resolved
rectools/models/ranking/candidate_ranking.py Outdated Show resolved Hide resolved
rectools/models/ranking/candidate_ranking.py Outdated Show resolved Hide resolved
rectools/models/ranking/candidate_ranking.py Outdated Show resolved Hide resolved
rectools/models/ranking/catboost_reranker.py Outdated Show resolved Hide resolved
tests/models/ranking/test_catboost_reranker.py Outdated Show resolved Hide resolved
tests/models/ranking/test_catboost_reranker.py Outdated Show resolved Hide resolved
tests/models/ranking/test_catboost_reranker.py Outdated Show resolved Hide resolved
olesyabulgakova and others added 10 commits January 30, 2025 11:54
Co-authored-by: Andrey Semenov <43339130+In48semenov@users.noreply.github.com>
Co-authored-by: Andrey Semenov <43339130+In48semenov@users.noreply.github.com>
Co-authored-by: Andrey Semenov <43339130+In48semenov@users.noreply.github.com>
Co-authored-by: Andrey Semenov <43339130+In48semenov@users.noreply.github.com>
Co-authored-by: Andrey Semenov <43339130+In48semenov@users.noreply.github.com>
Co-authored-by: Andrey Semenov <43339130+In48semenov@users.noreply.github.com>
Co-authored-by: Andrey Semenov <43339130+In48semenov@users.noreply.github.com>
Co-authored-by: Andrey Semenov <43339130+In48semenov@users.noreply.github.com>

def test_prepare_training_pool(
self,
reranker_catboost_ranker: CatBoostReranker,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move ranker and classifier to pytest.parametrize decorator for all of the tests where you need to test both approaches.
Here is an example on how to organize code when you have different outputs for different parameters:

@pytest.mark.parametrize(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Collaborator

@feldlime feldlime left a comment

Choose a reason for hiding this comment

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

Please fix these mistakes everywhere

rectools/models/ranking/candidate_ranking.py Outdated Show resolved Hide resolved
rectools/models/ranking/candidate_ranking.py Outdated Show resolved Hide resolved
rectools/models/ranking/candidate_ranking.py Outdated Show resolved Hide resolved
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.

4 participants