Bug #1997

Possible bug in probability normalization

Added by Benjamin van Niekerk 3 months ago. Updated about 1 month ago.

Status:NewStart date:2020-07-01
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

Hi,

I noticed a possible bug at https://code.soundsoftware.ac.uk/projects/pyin/repository/entry/YinUtil.cpp#L310 when normalizing the peakProb probabilities. When i = minInd, peakProb[minInd] gets normalized so for every i > minInd we multiply by a smaller value. As a result the final sum won't add to peakProb[minInd] and the probability that a frame is voiced would be lower than expected. However, this bug is also useful: if you run pYIN on a pure tone, for example, all the minima will have the same probability so the wrong pitch might be selected. This bug will reduce the probabilities for all minima after minInd.

History

#1 Updated by Chris Cannam 3 months ago

Interesting discovery, thanks for reporting it!

This certainly doesn't look intentional, but perhaps Matthias can weigh in. I'd love to know what difference it makes in terms of performance - I wonder if we can set up a test. Definitely not something we can just fix without testing, especially if, as you say, it might have benefits.

#2 Updated by Matthias Mauch about 1 month ago

Chris Cannam wrote:

Interesting discovery, thanks for reporting it!

This certainly doesn't look intentional, but perhaps Matthias can weigh in. I'd love to know what difference it makes in terms of performance - I wonder if we can set up a test. Definitely not something we can just fix without testing, especially if, as you say, it might have benefits.

Hi Benjamin, Chris,

I've looked into this a little, and I have a few observations

  • it does indeed seem to be a bug
  • the bug stems from a commit that was made in a hurry (by me) and it also includes some other big, ugly code changes that resulted in the implementation diverging from the pYIN algorithm as devised and tested in our paper

I think the right thing would be to re-instate the code that is currently commented out and actually reflects pYIN from the paper. I will see if I am allowed to do this given my current employment in industry. In the meantime, feel free to contact me via my private email.

@Benjamin: if you, by any chance, have code and ground truth lying round that would quickly allow you to assess the accuracy of any changes, that would be really handy. — I don't at the moment, but if I'm allowed to work on this I might try.

Cheers,
Matthias.

#3 Updated by Benjamin van Niekerk about 1 month ago

Thanks for the reponse Chris and Matthias.

As a bit of context, I discovered the bug when doing some comparisons against a version of pYIN I implemented for librosa. I'm guessing the goal of the changes implemented by Matthias was to allow pitch candidates other than the first minimum of the difference function? Based on this idea, the approach I went with in librosa was to put a prior over the position of the minimum. I used a discrete Boltzmann distribution that puts most of the probability mass on the first minimum and then less on successive minima. By changing the temperature of the distribution you can move between the two approaches: at one extreme you recover the implemetation described in the paper and at the other, you get a uniform distribution over all minima. In tests I ran on the MDB-stem-synth dataset this seemed to work quite well (see https://github.com/librosa/librosa/pull/1063#issuecomment-651042308 for example).

I'd be happy to do some benchmarking on MDB-stem-synth if that'd be helpful.

#4 Updated by Matthias Mauch about 1 month ago

Benjamin van Niekerk wrote:

Thanks for the reponse Chris and Matthias.

As a bit of context, I discovered the bug when doing some comparisons against a version of pYIN I implemented for librosa. I'm guessing the goal of the changes implemented by Matthias was to allow pitch candidates other than the first minimum of the difference function? Based on this idea, the approach I went with in librosa was to put a prior over the position of the minimum. I used a discrete Boltzmann distribution that puts most of the probability mass on the first minimum and then less on successive minima. By changing the temperature of the distribution you can move between the two approaches: at one extreme you recover the implemetation described in the paper and at the other, you get a uniform distribution over all minima. In tests I ran on the MDB-stem-synth dataset this seemed to work quite well (see https://github.com/librosa/librosa/pull/1063#issuecomment-651042308 for example).

I'd be happy to do some benchmarking on MDB-stem-synth if that'd be helpful.

Hi Benjamin,

I'm very impressed with the contribution you made, and the thread you pointed to is amazing. I've also thanked Brian separately. The Boltzmann distribution was in fact the thing that made me stumble most when looking at your implementation, so I'm glad I understand it now in principle.

Now that I understand your approach I'm even tempted to port it back to the Vamp implementation. But as a first step it'd be nice to simply tidy up there. I'll try to do that and also see if I can implement at least a clean switch between the two "extremes". Then, if I may, I would love to come back to your offer to do the benchmarking. (I do still really like the method as described in the paper — one of the more elegant things I've done — and it would be great to see how it does.)

Thank you for your work and explanation!
Matthias.

#5 Updated by Benjamin van Niekerk about 1 month ago

Hi Matthais,

Glad to have helped! Yeah, the pYIN paper is great and for the most part I've been setting the Boltzmann distribution temperature towards the direction described in the paper.

But as a first step it'd be nice to simply tidy up there. I'll try to do that and also see if I can implement at least a clean switch between the two "extremes".

That sounds good to me. Happy to do the benchmarking, just let me know when you're ready.

Also available in: Atom PDF