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

Batch vectorisation #28

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

giamic
Copy link
Contributor

@giamic giamic commented Mar 1, 2022

Allow users to pass batches of audio waveforms, vectorising the relevant code.

This adds flexibility to the code and also makes it faster to run on batched data. There are still probably a few points in which we could optimise even more, but this should do for a first iteration.

I have some tests on my local machine that seem to show that the output hasn't changed. They are not in the PR because I didn't want to keep two versions of every function I changed, it felt confusing and ultimately useless.

Unfortunately I'm not able to run the octave / matlab tests locally to verify if they still pass.

@giamic giamic marked this pull request as ready for review March 1, 2022 20:13
@giamic
Copy link
Contributor Author

giamic commented Mar 1, 2022

I would like to draw your attention to lines 178 and 185 of utils.py (https://github.com/mpariente/pystoi/pull/28/files#diff-add0854d17d77236abbfb630bbf6fabf6ed9a6745a06ff12b3cf89ec6a89ceb8L178). I have removed them because

  1. I don't think that they are necessary. Why would we add some noise before removing the average?
  2. Even if the addition of epsilon is necessary, why would we modulate it with a very expensive normal distribution call when the value of epsilon is such a tiny value?
  3. The tests seem to show that removing it doesn't change the value of STOI
  4. The code is ~40% faster when using extended STOI if we remove those two lines

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.

1 participant