Adding dockerfile

[esimkowitz]

I've written a dockerfile to build and run your pitch-detection code. As it's set up right now, it builds the code and its dependencies, as well as the test and benchmark programs.

One thing I noticed is that with the latest libraries from APT, the following four tests are failing. Is this a known issue? If so, do you know which versions of which libraries are stable with the code?

• PYinInstrumentTest.Piano_D4_44100

./test_instruments.cpp:157: Failure
The difference between expected and pitch is 568.59766233715936, which exceeds 0.01 * expected, where
expected evaluates to 293.69999999999999,
pitch evaluates to 862.2976623371593, and
0.01 * expected evaluates to 2.9369999999999998.

• PYinInstrumentTest.Acoustic_E2_44100

./test_instruments.cpp:166: Failure
The difference between expected and pitch is 82.858104120934797, which exceeds 0.01 * expected, where
expected evaluates to 82.409999999999997,
pitch evaluates to 165.26810412093479, and
0.01 * expected evaluates to 0.82409999999999994.

• MpmEdgeCase.InvalidAlloc

./test_edge_cases.cpp:18: Failure
Expected: pitch_alloc::Mpm<double> ma(-1) throws an exception of type std::bad_alloc.
  Actual: it throws a different type.

• YinEdgeCase.InvalidAlloc

./test_edge_cases.cpp:45: Failure
Expected: pitch_alloc::Yin<double> ya(-1) throws an exception of type std::bad_alloc.
  Actual: it throws a different type.]

[sevagh]

Thanks for the contribution. I'll leave some preliminary comments.

I'll take a look at those failing tests. The wrong pitches might be legitimate - no pitch detection method is perfect (and might be off by integer multiples for different harmonics), so those tests served to show the imperfection.

[sevagh]

I cloned your fork and was able to build the Dockerfile. So, OK, it works good. Getting the build dependencies in a container will be helpful for lots of users.

Would you like to add some usage text about the Dockerfile/docker-based build process you're envisioning? https://github.com/sevagh/pitch-detection#build-and-install

Another thought - this container is definitely useful for people compiling the code and easily linking against lpitch-detection. Maybe your help text can have an example for user, or some suggestion or convenient script to let them mount their own code into the container. E.g. "first write main.cpp, then docker run -v "main.cpp":/main.cpp..., ..."

[esimkowitz]

I cloned your fork and was able to build the Dockerfile. So, OK, it works good. Getting the build dependencies in a container will be helpful for lots of users.

Would you like to add some usage text about the Dockerfile/docker-based build process you're envisioning? https://github.com/sevagh/pitch-detection#build-and-install

Another thought - this container is definitely useful for people compiling the code and easily linking against lpitch-detection. Maybe your help text can have an example for user, or some suggestion or convenient script to let them mount their own code into the container. E.g. "first write main.cpp, then docker run -v "main.cpp":/main.cpp..., ..."

Sorry I had a busy week at work, looking at this now. I'll add a note about the Dockerfile. I think for the mounting I'm going to wait a bit to add that if that's okay with you. I haven't had time to actually write anything using the library yet, I can add something once I've made some more progress on my sample program.

[sevagh]

Looking good to me.

I'll most likely cut down on the README some time in the future after accepting this, to make it more terse (but keep the essence).

I won't let that block the PR though since debating writing style over GitHub will probably be exhausting.

[sevagh]

Can you squash your commits into 1 (https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History)? Message "Add Dockerfile" should be fine. If I squash from the GitHub UI, it might erase your authorship.

[esimkowitz]

Can you squash your commits into 1 (https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History)? Message "Add Dockerfile" should be fine. If I squash from the GitHub UI, it might erase your authorship.

Squashed! Never done that manually before!

[jsommr]

@sevagh, sorry to necropost, but I've been working on re-writing your code in C, where Yin was a no-brainer, but PYin is causing me a lot of headache. I'm wondering if the failing tests that esimkowitz mentions has something to do with it. Could you reproduce the issue? Because I'm getting the same failed tests and am wondering if something is "wrong" with the PYin version, or if you'd say it's working as expected?

[sevagh]

@jsommr

I just checked on this since your HN comment. I'll take a look soon. I have changed my C++ opinions dramatically since I last worked on pitch-detection, it's worth a new look. Also in pitchlite (https://github.com/sevagh/pitchlite/blob/main/src/shared.cpp) I have a slightly more efficient autocorrelation with real FFTs (and even more savings from not having std::complex vectors).

[sevagh]

I'm now addressing some of these errors:

MpmEdgeCase.InvalidAlloc
YinEdgeCase.InvalidAlloc

These used to throw std::bad_alloc but now they throw std::length_error - easy fix

[sevagh]

Regarding the unit tests for PYin, I added print statements for the pitch candidates in PYin:

(system) sevagh@pop-os:~/repos/pitch-detection$ ./test/test --gtest_filter="*PYinInstrumentTest*Piano_D4*"
Pitch candidate: 14192.7, 0.274923
Pitch candidate: 862.298, 0.395799
Pitch candidate: 294.535, 0.293946
Pitch candidate: 0.882388, 0.035329
./test_instruments.cpp:157: Failure
The difference between expected and pitch is 568.59766233715936, which exceeds 0.01 * expected, where
expected evaluates to 293.69999999999999,
pitch evaluates to 862.2976623371593, and
0.01 * expected evaluates to 2.9369999999999998.

(system) sevagh@pop-os:~/repos/pitch-detection$ ./test/test --gtest_filter="*PYinInstrumentTest*Acoustic_E2*"
Pitch candidate: 15592.7, 0.305056
Pitch candidate: 165.268, 0.413194
Pitch candidate: 82.6083, 0.137876
Pitch candidate: 21.5437, 0.143871
./test_instruments.cpp:166: Failure
The difference between expected and pitch is 82.858104120934797, which exceeds 0.01 * expected, where
expected evaluates to 82.409999999999997,
pitch evaluates to 165.26810412093479, and
0.01 * expected evaluates to 0.82409999999999994.

So, PYin in these cases is putting the highest confidence on the inappropriate pitch.

In the Piano D4 case, top 3 pitches are:

• 862, 0.4 (wrong)
• 294, 0.3 (correct)
• 14192, 0.27 (very wrong?)

In the Guitar E2 case, top 3 pitches are:

• 165, 0.41 (wrong)
• 15592, 0.3 (very wrong?)
• 82.6, 0.13 (correct)
• 21.54, 0.14 (wrong)

[sevagh]

Cool, here's one major fix of a bug:

[       OK ] PYinInstrumentTest.Piano_B4_44100 (51 ms)
[ RUN      ] PYinInstrumentTest.Piano_D4_44100
Pitch candidate: 14192.7, 0.00413251
Pitch candidate: 862.298, 0.631946
Pitch candidate: 294.535, 0.351047
./test_instruments.cpp:157: Failure
The difference between expected and pitch is 568.59766233715936, which exceeds 0.01 * expected, where
expected evaluates to 293.69999999999999,
pitch evaluates to 862.2976623371593, and
0.01 * expected evaluates to 2.9369999999999998.
[  FAILED  ] PYinInstrumentTest.Piano_D4_44100 (57 ms)
[ RUN      ] PYinInstrumentTest.Acoustic_E2_44100
Pitch candidate: 15592.7, 0.00413251
Pitch candidate: 165.268, 0.703553
Pitch candidate: 82.6083, 0.250371
Pitch candidate: 21.5437, 0.0290694
./test_instruments.cpp:166: Failure
The difference between expected and pitch is 82.858104120934797, which exceeds 0.01 * expected, where
expected evaluates to 82.409999999999997,
pitch evaluates to 165.26810412093479, and
0.01 * expected evaluates to 0.82409999999999994.
[  FAILED  ] PYinInstrumentTest.Acoustic_E2_44100 (6 ms)

The answers are still wrong, but it's much stronger for the likelier candidates than the outliers.

The bug is this:

-       T threshold = PYIN_MIN_THRESHOLD;
+       T threshold = 0.0f;

        for (int n = 0; n < PYIN_N_THRESHOLDS; ++n) {
-               threshold += n * PYIN_MIN_THRESHOLD;
+               threshold = (n + 1) * PYIN_MIN_THRESHOLD;

I think accumulating in the threshold is a bad error that results in huge thresholds for the highest frequencies, leading to some junk candidates having a very strong probability.

[sevagh]

I got all the tests passing with two more adjustments to the PYin code!

• Changed the Beta distribution to a different one (computed with a proper scipy beta distribution function, not re-using the same fake values from the implementation code)
• Changing the YIN threshold increment to 0.025 (i.e. testing thresholds 0.025, 0.05, 0.075, 0.1, ...) instead of 0.01 (testing 0.01, 0.02, 0.03, ...)

[jsommr]

Super awesome! Thanks for taking the time to investigate. I did have a hunch that those pesky beta distributions had something to do with it. Looking forward to translate it into C.

This library (non-GPL) uses a beta distribution with alpha 1.7 and a mean of .2 and seems to use some kind of dynamic threshold (find_valleys). Perhaps he's found a magical way - I'll have to test it after Christmas. Will return if something turns up.

[sevagh]

For now I'll settle for this code that gets my own unit tests passing, but that library looks like it has a good implementation. I'm using scipy to generate the beta distribution to hardcode in my code:

(system) sevagh@pop-os:~/repos/pitch-detection$ cat misc/generate_beta_distribution.py
import numpy as np
from scipy.stats import beta

# Define the parameters for the Beta distributions
# Example: mean = 0.1, beta_param = 18 (alpha is calculated from these)
means = [0.1, 0.15, 0.2]
beta_params = [18, 11 + 1/3, 8]  # Adjust these values as needed

# Number of points in the distribution
n_points = 100

for i, (mean, beta_param) in enumerate(zip(means, beta_params)):
    alpha = mean * beta_param / (1 - mean)
    x = np.linspace(0, 1, n_points)
    y = beta.pdf(x, alpha, beta_param)/100.0

    # Format as a C++ array
    cpp_array = ', '.join(f'{val:.6f}' for val in y)
    print(f"// Beta distribution with mean {mean} and beta {beta_param}\n"
          f"static const float Beta_Distribution_{i}[{n_points}] = "
          f"{{ {cpp_array} }};\n")

[sevagh]

Let's move the conversation here: #87

I'm going to address some long-standing issues, including the buggy sine wave generator, etc.