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

Fix broken save_audio #1147

Merged
merged 3 commits into from
Sep 14, 2023
Merged

Conversation

flyingleafe
Copy link
Contributor

@flyingleafe flyingleafe commented Sep 14, 2023

Commit 2046b55 introduced a silly yet horrible regression, completely breaking down save_audio method:

  • It stopped working in the first place for MixedCuts, since they do not have recording field, so fastcopy fails there;
  • It stopped really working for DataCuts as well, since fastcopy preserved the original start times of the cut, while the underlying recording had changed, and so every such cut became invalid after save_audio (except maybe for the one starting from the very beginning of the original recording.

I do not have time today to add regression test for this, maybe tomorrow.

This amounted to my almost entire working day being completely puzzled about why my data preparation pipeline got completely broken after I just changed the underlying data.... been blaming the data itself initially, not the master update.

(Basically, I did some .cut_into_windows + .padding + .filter and then .save_audios)

@pzelasko
Copy link
Collaborator

Good catch, sorry for that! I agree it should have a better test coverage. I'll merge it as-is to fix the broken behavior. Would you be willing to make a follow-up PR with tests?

@pzelasko pzelasko enabled auto-merge (squash) September 14, 2023 14:47
@pzelasko pzelasko merged commit 4d6d9c9 into lhotse-speech:master Sep 14, 2023
10 checks passed
@flyingleafe
Copy link
Contributor Author

@pzelasko If I have some spare time tomorrow, will do the test

@pzelasko pzelasko added this to the v1.17 milestone Oct 8, 2023
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.

None yet

2 participants