Update for near perfect correlation with pystoi
#9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpariente
reviewed
Apr 20, 2023
|
Also, feel free to change the graphs in the README within the PR, it's worth it 👀 |
Will do
This is inspired from mpariente/pystoi#28.
Done |
|
I guess you didn't yet commit the changes, but it all sounds good. |
|
No not yet, I am working on it. Turns out some fixes are required to run on CUDA. Will probably finish after the weekend. |
|
Done.
Edit: pressed "Comment" before finishing my message |
|
Thanks again, the benchmark is on GPU or CPU ? |
|
That last benchmark was on GPU, on a V100. |
|
What is the |
|
Which part of the code adds the most time ? |
|
The The benchmarking script can be found here. Namely the measured function is def to_time():
loss = criterion(x, y).mean()
loss.backward(retain_graph=True)where I can try and profile the code tomorrow and see what's taking the most time. I profiled on CPU and there it was the |
|
Also I ran this on a computing cluster and did not make sure both codes ran on the same host. I can try and fix that too tomorrow. |
|
Okay I made sure to run it again on the same host and processing is indeed about ~x2 times longer. Below are full master: this PR: |
mpariente
reviewed
May 1, 2023
Comment on lines
+328
to
+330
| return torch.stack([ | ||
| pad(xi[mi], (0, 0, 0, len(xi) - mi.sum())) for xi, mi in zip(x, mask) | ||
| ]) |
There was a problem hiding this comment.
This is the list comprehension which takes the most time, and it makes sense that it does.
|
Thanks for running the profiler ! I think a x2 in computation time still worth the improvement, specially that it will be negligible when compared to the NN's training time. |
|
I merged this PR in |
I also think it's worth the improvement. The time increase also happens inside
Sure. I will also try and think of a way to avoid this list comprehension. |
I don't think so. Backward through the loss is the first operation of the overall backward, the backward time of the whole architecture is independent of the loss, you just need the gradients as input. |
|
Latest commit now reflects mpariente/pystoi#33. I made new correlation plots and checked it is still strong. I can commit them if you want. Dunno if we should mention the correlation is against the latest commit in pystoi, since no new pystoi version was released after mpariente/pystoi#33. The correlation against current pystoi version i.e. pre-mpariente/pystoi#33 would not be as strong. |
Sure, add a commit tag somewhere in a comment.
Do you think the difference is that big ? |
|
Could you make a correlation plot between the former and new version of pySTOI please ? |
|
Okay the difference is very small so I guess it's fine. Correlation vs. new pystoi (should be 100% with VAD if it wasn't for the different resampling technique):
Correlation vs. old pystoi (i.e. pystoi now discards 1 or 2 frames at the end):
|
|
Thanks ! It's roughly the same, right ? |
|
Yes, I can't see the difference unless I superimpose the plots and keep Alt-Tabbing |
|
So please add a comment in the code, and the Readme, and we'll merge this. |
|
Done. Also updated the docstring to sound less dissuasive about using this vs. pystoi, while still emphasizing on the difference. Maybe you can have a look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR significantly improves correlations with
pystoiat little performance cost. Whenuse_vad=True, the correlation is near-perfect. The only reason it's not truly perfect is because of the different resampling technique. Changes are extensive and I am ready to discuss them with you.This also fixes two warnings:
torch.stftinNegSTOILoss.stftbecausereturn_complex=Falseis deprecatedtorchaudio.transforms.ResampleinNegSTOILoss.__init__becauseresampling_method=sinc_interpolationis deprecated and should now besinc_interp_hann.Correlations
Below are correlation plots obtained with this fix. These were obtained using mixtures I generated which I can provide if needed. As you can see when
use_vad=Truewe obtain near-perfect correlation. I did not include these plots in this PR because I didn't know if the same mixtures should be used to recreate the plots.For reference below is
masteraccording toREADME.mdvs. this PR w/ VAD. Quite an improvement IMO.Processing times
Below I compare processing times between
masterthis PR. The evaluated function is aNegSTOILoss.forwardpass followed by aloss.backwardpass where thenn.Moduleis theTestNetintest_torchstoi.py. Whenuse_vad=True, processing is a bit slower, but worth the accuracy improvement IMO. Withuse_vad=False, it seems it's actually slightly faster. These were obtained on a HP Elitebook G6 with an Intel(R) Core(TM) i7-8665U CPU @ 1.90GHz. I had no free GPU at hand when testing so I couldn't try on GPU.