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

Brings back original VAD parameters naming #1181

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Purfview
Copy link
Contributor

@Purfview Purfview commented Nov 29, 2024

For no reason in v1.1.0 VAD parameter threshold was changed to onset and neg_threshold to offset.

This PR brings original naming back.

For no reason in v1.1.0 VAD parameter `threshold` was changed to `onset`  and `neg_threshold` to `offset`. 

This PR brings reverts that.
Copy link
Collaborator

@MahmoudAshraf97 MahmoudAshraf97 left a comment

Choose a reason for hiding this comment

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

That is more inline with original silero naming

@MahmoudAshraf97
Copy link
Collaborator

As for the failing CI, tokenizers dropped support for python 3.8 in the latest release but due to their wheel structure it will still be installed if you have 3.8, would you suggest adding a requirement to constraint the version or should we drop python 3.8 support and switch CI to 3.9? There is nothing to drop essentially, but we will not be maintaining the requirements that will support 3.8 out of the box. The reason I'm inclined to the second option is that tokenizers is a widely used library and sooner or later faster whisper will start to clash with other projects that require a higher version than what faster whisper needs.
Python 3.8 is EOL since the last month btw.

@Purfview
Copy link
Contributor Author

Purfview commented Nov 29, 2024

That is more inline with original silero naming

What's more important that it's inline with pre-1.1.0 faster-whisper, renaming parameters breaks many projects for no reason.

As for the failing CI, tokenizers dropped support for python 3.8 in the latest release...

Just switch CI to 3.9 I think, anyway, savvy users will be able to solve compatibility issues, I doubt that newbies use 3.8. :)

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