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

[BC-Breaking] Remove compute_kaldi_pitch #3368

Closed
wants to merge 1 commit into from

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented May 24, 2023

This commit removes compute_kaldi_pitch function and the underlying Kaldi integration from torchaudio.

Kaldi pitch function was added in a short period of time by integrating the original Kaldi implementation, instead of reimplementing it in PyTorch.

The Kaldi integration employed a hack which replaces the base vector/matrix implementation of Kaldi with PyTorch Tensor so that there is only one blas library within torchaudio.

Recently, we are making torchaudio more lean, and we don't see a wide adoption of kaldi_pitch feature, so we decided to remove them.

See some of the discussion #1269

@pytorch-bot
Copy link

pytorch-bot bot commented May 24, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/audio/3368

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures

As of commit 0db6e14:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@mthrok mthrok force-pushed the remove-kaldi-integration branch 2 times, most recently from eb4dcea to b7a34e7 Compare May 24, 2023 13:02
@vadimkantorov
Copy link

vadimkantorov commented May 27, 2023

Also, could make sense to just create a GitHub feature request issue in torchaudio for some pitch extraction algos (which could be useful for TTS?), and later on add a new function reimplemented from the ground up without any kaldi-compat as a goal

@mthrok mthrok force-pushed the remove-kaldi-integration branch from b7a34e7 to 0102f69 Compare May 31, 2023 23:10
@mthrok
Copy link
Collaborator Author

mthrok commented Jun 1, 2023

Also, could make sense to just create a GitHub feature request issue in torchaudio for some pitch extraction algos (which could be useful for TTS?), and later on add a new function reimplemented from the ground up without any kaldi-compat as a goal

Yeah, that makes sense, however I don't know much about what kind of pitch extraction ppl are using these days, so I don't know what are the good candidates. I also hear that people use NN-based pitch extraction. In that case, it requires more work to add such.

This commit removes compute_kaldi_pitch function and the underlying
Kaldi integration from torchaudio.

Kaldi pitch function was added in a short period of time by
integrating the original Kaldi implementation, instead of
reimplementing it in PyTorch.

The Kaldi integration employed a hack which replaces the base
vector/matrix implementation of Kaldi with PyTorch Tensor so that
there is only one blas library within torchaudio.

Recently, we are making torchaudio more lean, and we don't see
a wide adoption of kaldi_pitch feature, so we decided to remove them.

See some of the discussion pytorch#1269
@mthrok mthrok force-pushed the remove-kaldi-integration branch from 0102f69 to 0db6e14 Compare June 2, 2023 04:08
@mthrok mthrok marked this pull request as ready for review June 2, 2023 20:15
@facebook-github-bot
Copy link
Contributor

@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mthrok merged this pull request in 5bbbb1d.

@mthrok mthrok deleted the remove-kaldi-integration branch June 3, 2023 02:03
mthrok added a commit to mthrok/audio that referenced this pull request Jun 3, 2023
Follow up of: pytorch#3368

Remove files and lines no longer used.
facebook-github-bot pushed a commit that referenced this pull request Jun 5, 2023
Summary:
Follow up of: #3368

Remove files and lines no longer used.

Pull Request resolved: #3403

Differential Revision: D46441462

Pulled By: mthrok

fbshipit-source-id: 11b881ec4b24fa0d625c6aee9f4bd91f637f9923
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants