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

PitchYIN error on stationary signals #230

Closed
faroit opened this issue Mar 3, 2015 · 15 comments
Closed

PitchYIN error on stationary signals #230

faroit opened this issue Mar 3, 2015 · 15 comments
Labels
Milestone

Comments

@faroit
Copy link

faroit commented Mar 3, 2015

Hello

lately we used the YIN implementation in essentia a lot. However for many applications (speech, instruments) I found an constant error compared to other pitch estimators like RAPT.

I tried to produce some more systematic results by running a simple test script (https://gist.github.com/faroit/2ebcf956633f63d92ace) which generates a stationary sine wave of constant f0. The signal then is processed by the YIN algorithm and the mean of the estimate is compared to the (constant) ground truth.

This is what I get:

yin_error

Obviously the estimation error is frequency depended, which is expected. Over 1 Khz, however, the estimate looks to be unstable.

Did anyone have tested the estimate in comparison to the original C Yin implementation?

@dbogdanov
Copy link
Member

PitchYinFFT is implemented based on the PhD thesis by Paul Brossier, in which he did provide evaluation details (see the thesis, page 91), but we did not carry out any further in-house evaluations. Looking at those graphs, we can expect YinFFT to work worse than Yin on lower notes.

It will be so great to compare the plot you made for YinFFT to the one for the original Yin. I wonder if you are interested in running this evaluation. This will be very useful, as we are trying to understand if it's really worth implementing original Yin.

@faroit
Copy link
Author

faroit commented Mar 5, 2015

Sure, I can make this comparison. Might happen next week.

@oriolromani
Copy link
Contributor

I made a comparison using http://aubio.org/ and a script based on the one used by @faroit.

pitch

It looks like the Yin is much better than its frequency-based version for lower frequencies, although the aubio implementation gives very bad result for the very first frames.
I think faroit is using this implementation. We'll see if it gives similar results

@faroit
Copy link
Author

faroit commented Mar 10, 2015

I compared with a CFFI interface to https://code.google.com/p/pysnail/ which is based on a modified version of fixed point implementation that is used here, as @oriolromani guessed. It's not even worth showing the results. They are a lot more unstable than essentia and the version interfaced in aubio and have jumps all over the place.

@faroit
Copy link
Author

faroit commented Mar 10, 2015

Next up could be: pYin which includes a Yin implementation. I will try to interface it soon using CFFI. Other than that there is a bunch of MATLAB implementations but I'm sick of those matlabwrappers ;-)

@oriolromani
Copy link
Contributor

I've tried the pYin Vamp plugin in Sonic Visualizer and it gives pretty good results for low frequencies but I still don't have comparable results.

@dbogdanov
Copy link
Member

It looks like the Yin is much better than its frequency-based version for lower frequencies, although the aubio implementation gives very bad result for the very first frames.

So, the implementation of Yin in aubio looks like the most promising. Are bad results in the first frames due to the specifics of how Yin algorithm works?

@dbogdanov
Copy link
Member

The script in gist is not entirely correct, you should use FrameGenerator with startFromZero=True, othewise the first frame is centered aroudn zero-th sample

@dbogdanov
Copy link
Member

I have added a new PitchYin algorithm (a1cd075). It works already although results are not that good in higher frequencies compared to the plot by @oriolromani

@faroit
Copy link
Author

faroit commented Mar 11, 2015

@dbogdanov thats right, thanks. (I updated the gist).
I'm still working on interfacing the pYin so we can have an ultimate yin shootout.

@dbogdanov
Copy link
Member

Finished implementing PitchYin. Here are the evaluation results agains PitchYinFFT (using this test script: https://gist.github.com/dbogdanov/a7b78caa7c2df5cf45f3). Looks pretty good.

pitchyin_vs_pitchyinfft

@andimarafioti
Copy link

Hi @dbogdanov, when I try to implement PitchYin i get the following error
AttributeError: 'module' object has no attribute 'PitchYin'
I am trying to implement the Inharmonicity module and I need to detect the pitch beforehand. Do you have any idea why python can't find this module? It works fine with PitchYinFFT.

@dbogdanov
Copy link
Member

It is in the master branch. You'll need to do ./waf install in order to make this algorithm available in python.

@dbogdanov dbogdanov added this to the 2.1 milestone May 21, 2015
@dbogdanov
Copy link
Member

Unit tests for PitchYin are not finished yet, we still need to make sure it all works ok.

@dbogdanov dbogdanov added the bug label May 21, 2015
@dbogdanov
Copy link
Member

Finished unit tests and did some fixes in the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants