marl / crepe

CREPE: A Convolutional REpresentation for Pitch Estimation -- pre-trained model (ICASSP 2018)
https://marl.github.io/crepe/
MIT License
1.11k stars 158 forks source link

Bug in Viterbi decoding? #37

Open justinsalamon opened 5 years ago

justinsalamon commented 5 years ago

On core.py#L149 we're passing the argmax of pitch salience matrix as observations to the decoder, but shouldn't we be passing the salience matrix directly? <-- @jongwook

jongwook commented 5 years ago

In that case we'd need a continuous emission probability model. For continuous emissions hmmlearn only supports Gaussian / Gaussian mixtures. We could of course use one of those instead of the current multinomial distribution; I mostly followed the pYIN paper (eqn 6) for defining the emission probability. For continuous emission we'll need to come up with a new heuristic to define our prior.

I thought about this before, and I wanted to use a (scaled) Dirichlet or element-wise Beta distribution because the support is constrained in [0, 1], but hmmlearn does not support them; that was where I previously gave up implementing the continuous emission.

maxrmorrison commented 5 years ago

Hi @jongwook. I think my discussions with @justinsalamon today prompted this issue. I used librosa.sequence.viterbi to decode discrete emissions directly from the posterior observations emitted by the network. I tried the same transition matrix you propose. I found that this significantly reduced the number of half/double frequency errors I was getting when working with speech data. The output pitch is still quantized, but can be smoothed or dithered to reduce binning effects if needed.

jongwook commented 5 years ago

@maxrmorrison Thanks for the context! I haven't thought in that way, i.e. interpreting the salience matrix as the observation probability distribution, but it does look like a better way to utilize the full salience matrix.

Are you normalizing each column to sum to 1? There's still some theoretical gap because the salience is obtained using multi-label binary cross-entropy instead of softmax, but if it works in practice, we should totally use it.

hmmlearn requires us to explicitly provide the emission probability distribution p(obs|state) and the observation sequence obs: timestep -> state, while librosa accepts the collapsed p(state|timestep) disregarding the observation space. The latter works just fine when we are predicting p(state|timestep) directly.

I'll welcome a PR that replaces hmmlearn with librosa.sequence.viterbi.

maxrmorrison commented 5 years ago

A PR has been started! Regarding the theoretical justification, yes: the output of softmax (the posterior distribution) should be used and not the raw logits. As the network was trained to minimize the distance between the categorical posterior distribution and the (also categorical), it is reasonable to decode directly from this distribution rather than after an additional sampling process (e.g., argmax).

justinsalamon commented 5 years ago

@maxrmorrison @jongwook Delighted to see this moving forward!

One quick question - librosa is a pretty heavy dependency. Could we just carve out just the viterbi decoding code, so as to avoid having the entire library as a dependency?