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

Improve SegmentSeeker word alignment #305

Merged
merged 27 commits into from
Feb 22, 2025

Conversation

ZachNagengast
Copy link
Contributor

These changes primarily focus on enhancing the SegmentSeeker class to fix some edge cases and make it more testable. It also adds public initializers to various structs and uses more flexible dependency versioning to allow usage of newer versions of swift-transformers if desired.

@ZachNagengast ZachNagengast requested a review from a2they February 21, 2025 10:34
Copy link
Contributor

@a2they a2they left a comment

Choose a reason for hiding this comment

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

lgtm

}

public init(
id: Int = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

since we set defaults here, we should remove redundancy above

return updatedSegments
}

public func calculateWordDurationConstraints(alignment: [WordTiming]) -> (Float, Float) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add names to Floats in the return tuple, i.e. (median: Float, max: Float)

let end = (timeOffset + timing.end).rounded(2)

// Handle short duration words by moving their start time back if there's space
if end - start < constrainedMedianDuration / 4 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it would be nice to add example in comments for each of these if/else cases

@ZachNagengast ZachNagengast linked an issue Feb 21, 2025 that may be closed by this pull request
@ZachNagengast ZachNagengast merged commit ca49596 into main Feb 22, 2025
13 checks passed
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.

Updating swift-transformer
2 participants