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: Set default window size to 20 #145

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pjyi2147
Copy link

Pull Request Checklist

Reference Issue

Please provide the reference to issue this PR is addressing (# followed by the issue number). If there is no associated issue, write "N/A".

ref: #144

Checklist Items

Before submitting your pull request, please review these items:

  • Have you followed the contributing guidelines?
  • Have you verified that there are no existing Pull Requests for the same update/change?
  • Have you updated any relevant documentation or added new tests where needed?

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Documentation content changes
  • Other...
    • Description:

@pjyi2147
Copy link
Author

pjyi2147 commented Sep 29, 2024

Another suggestion:
Once a paremeter defined in **kwargs in any way (even None), it is no use of setting default values in kwargs.get().
It would be more clear to remove default values in kwargs.get() and let it fail if similar error occurs.

Example code for top_k_retrieve and window_size.

They are defined in run_rank_llm.py, therefore, no default values are required.

# rank_listwise_os_llm.py:116-117
top_k_retrieve: int = kwargs.get("top_k_retrieve", 50)
window_size: int = kwargs.get("window_size", 20)

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.

1 participant