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

Fix AWGN resampler test #105

Closed
dshil opened this issue Aug 28, 2017 · 8 comments
Closed

Fix AWGN resampler test #105

dshil opened this issue Aug 28, 2017 · 8 comments
Assignees
Labels
category: tests Task for writing or improving tests
Milestone

Comments

@dshil
Copy link
Member

dshil commented Aug 28, 2017

The result of last rebase on @baranovmv changes.

     TEST   roc_audio
........................
src/tests/roc_audio/test_resampler.cpp:159: error: Failure in TEST(resampler, upscaling_twice_awgn)
	CHECK(buff[i] >= -60) failed

..!!......................
.
Errors (1 failures, 51 tests, 49 ran, 147101 checks, 2 ignored, 0 filtered out, 219 ms)

scons: *** [test/roc_audio] Error 1
@gavv gavv added the bug label Aug 28, 2017
@gavv gavv added this to the 0.1 milestone Aug 28, 2017
@baranovmv
Copy link
Member

Wow, that's difficult. especially this code works fine on PC.
I always add some code in tests or resampler to store the signal in a file so as to be able to look at it, but I don't have access to a Mac now. If I won't manage to get a Mac, we can leave it as is, until we meet (hope it'll happen this year).

@gavv
Copy link
Member

gavv commented Aug 28, 2017

We could add an option (e.g. ifdef) that dumps the necessary data to a file for such cases.

@gavv
Copy link
Member

gavv commented Sep 19, 2017

Since the hardware architecture and compiler are the same, I can only imagine a bug in memory management in our code or some difference is stdlib functions, like random or math.

@dshil
Copy link
Member Author

dshil commented Sep 20, 2017

ping @baranovmv,

I've simply put several print to debug a problem:

TEST(resampler, upscaling_twice_awgn) {
    enum { ChMask = 0x1 };

    MockReader reader;
    Resampler resampler(reader, buffer_pool, allocator, config, ChMask);

    CHECK(resampler.set_scaling(0.5f));

    const size_t sig_len = 2048;
    double buff[sig_len * 2];
    size_t i;

    for (size_t n = 0; n < InSamples; n++) {
        const sample_t s = (sample_t)AWGN_generator();
        reader.add(1, s);
    }

    // Put the spectrum of the resampled signal into buff.
    // Odd elements are magnitudes in dB, even elements are phases in radians.
    get_sample_spectrum1(resampler, buff, sig_len);

    for (i = 0; i < sig_len - 1; i += 2) {
        if (i <= sig_len * 0.90 / 2) {
            if (buff[i] < -60) {
                printf("i = %lu, buf[i] = %f\n", i, buff[i]);
            }
            /* CHECK(buff[i] >= -60); */
        } else if (i >= sig_len * 1.00 / 2) {
            if (buff[i] > -60) {
                printf("i = %lu, buf[i] = %f\n", i, buff[i]);
            }
            /* CHECK(buff[i] <= -60); */
        }
    }
}

And as a result:

i = 518, buf[i] = -74.226430

i = 1024, buf[i] = -59.682005
i = 1026, buf[i] = -59.800069
i = 1028, buf[i] = -59.847236
i = 1030, buf[i] = -59.815538
i = 1032, buf[i] = -59.871927
i = 1036, buf[i] = -59.982135

The whole buff

A glimmer of hope: Does it tell something to you ? :)

gavv pushed a commit that referenced this issue Oct 7, 2017
test_resampler.cpp: (TEST(resampler, upscaling_twice_awgn)) was skipped,
because of a regular failure. There is a little magic here, because this
test is passed on all other target platforms.

Related: #105
@gavv gavv removed this from the 0.1 milestone Dec 7, 2017
@baranovmv
Copy link
Member

  1. as this test was committed, then I suppose it passes on PCs under Linux, right? Then the issue relates somehow with compiler and/or standard libraries, e.g. different floating point math implementation, etc...
  2. according to the numbers, the spectrum is a little bit above the border (~0.5 dB). The border -60 dB should have been chosen with some margin, so it might be a marker of some hidden troubles. Or it can be just a random difference in implementation.

I am not able to reproduce this bug, sorry. I suggest roc to hold this issue until someone will refactor the Resampler.

@gavv gavv changed the title roc_audio tests failed on OS X Resampler test fails on OS X May 30, 2019
@gavv gavv modified the milestones: 0.2, 0.1, 0.1.0, 0.1.1 May 30, 2019
@gavv gavv self-assigned this Jun 9, 2019
@gavv
Copy link
Member

gavv commented Jun 9, 2019

It appeared that this test was just broken in several aspects and the problem was not specific to macOS.

I was able to reproduce this on Linux too, by setting non-default random seed, e.g. adding srand(100500) to the beginning of the test. It just happened that it worked with the default seed on Linux and didn't work with the default seed on macOS.

The test has several problems:

  • The test is not truly random because it always uses the default seed for rand().

  • The test's requirement for the lower half of the spectrum to be above -60dB is not strictly met on any generated noise. The spectrum may have spikes if some frequencies are mostly missing, which happens from time to time when we use random seed. The spectrum value for such frequencies may be below -70dB, for example.

  • The test's requirements for the upper half of the spectrum to be below -60dB is not always met as well. My tests showed that we can only rely on -50dB.

  • The test's requirements for the cut-off border between the lower and the upper halves (90%) was too strict as well.

  • The check for the upper half of the spectrum was incorrect. The condition if (i >= sig_len * 1.00 / 2) was never triggered.

What I've done (3f62f7a):

  • Switched AWGN generator from stdlib rand() to Roc core::random(), which is guaranteed to have a random seed. After doing this, the test started failing on Linux frequently.

  • Applied a median filter to the spectrum to remove spikes caused by missing frequencies.

  • Relaxed and fixed test's requirements both for the magnitude and for the cut-off border.

  • Refactored AWGN generator. Its code was overcomplicated and the comments were wrong.

The test is now stable both on Linux and macOS.

@baranovmv The 90% cut-off border and -60dB magnitude threshold were too optimistic for our resampler with this AWGN generator. I don't know where did the specific requirements come from. For now my goal is to make the test correct and stable. Then we can think whether the relaxed requirement are okay or we need some changes in the resampler or in the test.

UPD: after a short discussion, it seems that the relaxed requirements are not great. I think we need to perform more testing on different resampler profiles and inputs.

@gavv gavv changed the title Resampler test fails on OS X Fix AWGN resampler test Jun 9, 2019
@gavv gavv added category: tests Task for writing or improving tests and removed defect labels Jun 9, 2019
@gavv
Copy link
Member

gavv commented Jun 9, 2019

Here are a few examples of the spectrums that didn't fit the original test requirements.

Spikes in the lower half

s1

Higher magnitude in the upper half

(spikes were removed using a median filter)

s2

s3

A bit shifted cut-off border

(spikes were removed using a median filter)

s4

@gavv gavv mentioned this issue Jun 9, 2019
@gavv
Copy link
Member

gavv commented Jun 9, 2019

Closing this. We can discuss further action in #153.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tests Task for writing or improving tests
Projects
Status: Done
Development

No branches or pull requests

3 participants