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 flaky fm_detect test #715

Merged
merged 1 commit into from
Dec 10, 2023
Merged

Fix flaky fm_detect test #715

merged 1 commit into from
Dec 10, 2023

Conversation

argilo
Copy link
Member

@argilo argilo commented Dec 9, 2023

volk_32fc_s32f_magnitude_16i fails in i386 Debian CI and locally:

68: offset 763 in1: 0.0534787 in2: 0.0534787 tolerance was: 1e-06
68: offset 4405 in1: -0.0228593 in2: -0.0228593 tolerance was: 1e-06
68: offset 6398 in1: -0.0512945 in2: -0.0512946 tolerance was: 1e-06
68: offset 8635 in1: -0.0217852 in2: -0.0217853 tolerance was: 1e-06
68: offset 9565 in1: 0.0521163 in2: 0.0521164 tolerance was: 1e-06
68: offset 10353 in1: 0.0130416 in2: 0.0130415 tolerance was: 1e-06
68: offset 14364 in1: -0.0432156 in2: -0.0432155 tolerance was: 1e-06
68: offset 17398 in1: 0.0538341 in2: 0.0538342 tolerance was: 1e-06
68: offset 17866 in1: 0.0584753 in2: 0.0584753 tolerance was: 1e-06
68: offset 25493 in1: 0.0356272 in2: 0.0356271 tolerance was: 1e-06
68: volk_32f_x2_fm_detectpuppet_32f: fail on arch a_avx
68: offset 763 in1: 0.0534787 in2: 0.0534787 tolerance was: 1e-06
68: offset 4405 in1: -0.0228593 in2: -0.0228593 tolerance was: 1e-06
68: offset 6398 in1: -0.0512945 in2: -0.0512946 tolerance was: 1e-06
68: offset 8635 in1: -0.0217852 in2: -0.0217853 tolerance was: 1e-06
68: offset 9565 in1: 0.0521163 in2: 0.0521164 tolerance was: 1e-06
68: offset 10353 in1: 0.0130416 in2: 0.0130415 tolerance was: 1e-06
68: offset 14364 in1: -0.0432156 in2: -0.0432155 tolerance was: 1e-06
68: offset 17398 in1: 0.0538341 in2: 0.0538342 tolerance was: 1e-06
68: offset 17866 in1: 0.0584753 in2: 0.0584753 tolerance was: 1e-06
68: offset 25493 in1: 0.0356272 in2: 0.0356271 tolerance was: 1e-06
68: volk_32f_x2_fm_detectpuppet_32f: fail on arch a_sse
68: offset 763 in1: 0.0534787 in2: 0.0534787 tolerance was: 1e-06
68: offset 4405 in1: -0.0228593 in2: -0.0228593 tolerance was: 1e-06
68: offset 6398 in1: -0.0512945 in2: -0.0512946 tolerance was: 1e-06
68: offset 8635 in1: -0.0217852 in2: -0.0217853 tolerance was: 1e-06
68: offset 9565 in1: 0.0521163 in2: 0.0521164 tolerance was: 1e-06
68: offset 10353 in1: 0.0130416 in2: 0.0130415 tolerance was: 1e-06
68: offset 14364 in1: -0.0432156 in2: -0.0432155 tolerance was: 1e-06
68: offset 17398 in1: 0.0538341 in2: 0.0538342 tolerance was: 1e-06
68: offset 17866 in1: 0.0584753 in2: 0.0584753 tolerance was: 1e-06
68: offset 25493 in1: 0.0356272 in2: 0.0356271 tolerance was: 1e-06
68: volk_32f_x2_fm_detectpuppet_32f: fail on arch u_avx

There are two bugs:

  1. The test uses a relative tolerance, but the outputs are differences of two potentially large values. If the difference happens to be near zero, the test fails.
  2. If the difference between two inputs happens to be close to bound or -bound (both of which represent a phase difference of 180 degrees), then a small floating point error can flip the sign of the output value, causing the test to fail.

To fix the first problem, I've switched the test to an absolute tolerance, and to fix the second I've increased the bound to 2.0. After these changes, the test passes reliably on i386.

Signed-off-by: Clayton Smith <argilo@gmail.com>
@argilo argilo mentioned this pull request Dec 9, 2023
Copy link
Contributor

@jdemel jdemel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jdemel jdemel merged commit f210701 into gnuradio:main Dec 10, 2023
32 checks passed
Alesha72003 pushed a commit to Alesha72003/volk that referenced this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants