-
Notifications
You must be signed in to change notification settings - Fork 223
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 for trim_to_alignments
issue
#1193
Conversation
@@ -363,15 +363,15 @@ def test_cut_trim_to_supervisions_keep_overlapping_extend(mono_cut): | |||
# Extended on the right side only by (4.0 - 3.37) / 2 == 0.315; | |||
# the left side is capped by the start of the recording. | |||
assert len(c1.supervisions) == 1 | |||
assert c1.start == 0.0 | |||
assert c1.start == 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After changing the mono_cut
fixture, this test case no longer represents the illustrated scenario. Also the comment above seems to be in conflict with the test outcome (looks like c1
should have 4 seconds now that it's not capped by the start of the recording anymore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll fix the tests tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I lost track of this. Have made the appropriate changes now --- should not affect the older tests.
Ref: #1186
The key issue was that in a couple of places, we did not account for the fact that start time of
AlignmentItem
is w.r.t. the start of the recording, whereas that forSupervisionSegment
is w.r.t. the Cut.with_offset
function for the supervision, alignment start times do not change, since they are w.r.t. the recording.