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

Added implementation for SuppressTokensFilter #14

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

jkrukowski
Copy link
Contributor

  • added implementation for SuppressTokensFilter
  • added tests

If this PR looks good, I can try to implement other filters 🫡

@ZachNagengast
Copy link
Contributor

Awesome contribution! Reviewing this shortly

Copy link
Contributor

@ZachNagengast ZachNagengast left a comment

Choose a reason for hiding this comment

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

This looks great @jkrukowski just a couple small notes, will run through the full tests as well.

Sources/WhisperKit/Core/Models.swift Outdated Show resolved Hide resolved
Tests/WhisperKitTests/UnitTests.swift Outdated Show resolved Hide resolved
Sources/WhisperKitCLI/transcribe.swift Outdated Show resolved Hide resolved
Tests/WhisperKitTests/UnitTests.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@ZachNagengast ZachNagengast left a comment

Choose a reason for hiding this comment

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

Updates look good, everything is passing, well done!

@ZachNagengast
Copy link
Contributor

Would love to see your take on the other filters if you're up for it! I'm going to be looking at word-level timestamps next on my side FYI, so we'll definitely need timestamp filtering i.e. ApplyTimestampRules equivalent for us, but keep in mind some of this logic we pulled into the SegmentSeeking protocol.

@jkrukowski
Copy link
Contributor Author

Would love to see your take on the other filters if you're up for it! I'm going to be looking at word-level timestamps next on my side FYI, so we'll definitely need timestamp filtering i.e. ApplyTimestampRules equivalent for us, but keep in mind some of this logic we pulled into the SegmentSeeking protocol.

sure, happy to do it. Will try to find some time this or next week

@ZachNagengast ZachNagengast merged commit d25c61b into argmaxinc:main Feb 5, 2024
2 checks passed
@jkrukowski jkrukowski deleted the supress-tokes branch February 5, 2024 19:00
@jkrukowski
Copy link
Contributor Author

jkrukowski commented Feb 12, 2024

Would love to see your take on the other filters if you're up for it! I'm going to be looking at word-level timestamps next on my side FYI, so we'll definitely need timestamp filtering i.e. ApplyTimestampRules equivalent for us, but keep in mind some of this logic we pulled into the SegmentSeeking protocol.

hi @ZachNagengast, not sure if it's a good place to ask but you mentioned that some of this logic was pulled into SegmentSeeking protocol. Can you please elaborate a bit more? (I'm looking into implementing TimestampRulesFilter based on ApplyTimestampRules)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants