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

change language_detection_threshold type to float #1188

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

Conversation

extrange
Copy link

@extrange extrange commented Dec 4, 2024

language_detection_threshold can't be supplied as None as detect_language compares it with a float.

Change parameter type in WhisperModel to reflect this.

Added test to confirm this behavior.

@MahmoudAshraf97
Copy link
Collaborator

MahmoudAshraf97 commented Dec 4, 2024

Hello, what is the use case where it needs to be supplied as None?

@extrange
Copy link
Author

extrange commented Dec 4, 2024

There's no use case for None, so the Optional has been removed. Furthermore, supplying None raises a TypeError as the test shows. So, we shouldn't allow None to be supplied for type safety.

@MahmoudAshraf97
Copy link
Collaborator

I see your point, thanks for noticing
for consistency can you also review other occurrences of this case? these are some examples

language_detection_threshold: Optional[float] = 0.5,

also I don't think the test is needed, the number of invalid input combos is too large to have tests for all of them, what do you think?

@extrange
Copy link
Author

extrange commented Dec 4, 2024 via email

Cannot be an optional parameter
@kenho211
Copy link

kenho211 commented Jan 8, 2025

Hello, what is the use case where it needs to be supplied as None?

From openai/whisper#676, the use case of None should be to conduct a majority vote for however many segments allowed to determine the top language.
For the current implementation, we can use language_detection_threshold = 1 to do the same.

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.

3 participants