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

filter: Accept rounded output from fir_filter_fsf #6980

Merged
merged 1 commit into from Dec 2, 2023

Conversation

argilo
Copy link
Member

@argilo argilo commented Dec 2, 2023

Description

VOLK recently accepted gnuradio/volk#709, which fixes truncate-toward-zero distortion in volk_32f_x2_dot_prod_16i. Unfortunately, this causes test failures:

======================================================================
FAIL: test_fir_filter_fsf_001 (__main__.test_filter)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/argilo/prefix_311/src/gnuradio/gr-filter/python/filter/qa_fir_filter.py", line 173, in test_fir_filter_fsf_001
    self.assertComplexTuplesAlmostEqual(expected_data, result_data, 5)
  File "/home/argilo/prefix_311/src/gnuradio/gnuradio-runtime/python/gnuradio/gr_unittest.py", line 73, in assertComplexTuplesAlmostEqual
    return all([
  File "/home/argilo/prefix_311/src/gnuradio/gnuradio-runtime/python/gnuradio/gr_unittest.py", line 74, in <listcomp>
    self.assertComplexAlmostEqual(x, y, places, msg)
  File "/home/argilo/prefix_311/src/gnuradio/gnuradio-runtime/python/gnuradio/gr_unittest.py", line 37, in assertComplexAlmostEqual
    raise self.failureException(
AssertionError: 1 != 2 within 5 places

======================================================================
FAIL: test_fir_filter_fsf_002 (__main__.test_filter)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/argilo/prefix_311/src/gnuradio/gr-filter/python/filter/qa_fir_filter.py", line 188, in test_fir_filter_fsf_002
    self.assertComplexTuplesAlmostEqual(expected_data, result_data, 5)
  File "/home/argilo/prefix_311/src/gnuradio/gnuradio-runtime/python/gnuradio/gr_unittest.py", line 73, in assertComplexTuplesAlmostEqual
    return all([
  File "/home/argilo/prefix_311/src/gnuradio/gnuradio-runtime/python/gnuradio/gr_unittest.py", line 74, in <listcomp>
    self.assertComplexAlmostEqual(x, y, places, msg)
  File "/home/argilo/prefix_311/src/gnuradio/gnuradio-runtime/python/gnuradio/gr_unittest.py", line 37, in assertComplexAlmostEqual
    raise self.failureException(
AssertionError: 5 != 6 within 5 places

This is because the tests in question assert the truncate-toward-zero behaviour in fir_filter_fsf. I would consider this behaviour buggy, and I would be surprised if there's any code that depends on fir_filter_fsf working this way. So I propose to change the tests to assert rounded output, with a temporary fallback to truncated output which can be removed once GNU Radio requires VOLK >= 3.1.0.

Which blocks/areas does this affect?

  • Decimating FIR Filter (Float->Short variant only)
  • Interpolating FIR Filter (Float->Short variant only)

Testing Done

I verified that the updated tests pass before & after the VOLK change.

Checklist

Signed-off-by: Clayton Smith <argilo@gmail.com>
@argilo argilo added filter Bug Fix VOLK Backport-3.10 Should be backported to 3.10 labels Dec 2, 2023

# TODO: Remove fallback once GNU Radio's required VOLK_VERSION is >= 3.1.0
try:
self.assertEqual(expected_data, result_data)
Copy link
Member Author

Choose a reason for hiding this comment

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

I also changed the assertions from assertComplexTuplesAlmostEqual to assertEqual, since the outputs are integers, not complex numbers.

@argilo
Copy link
Member Author

argilo commented Dec 2, 2023

The test failures are unrelated; they're both due to #6981.

@willcode willcode merged commit c297a62 into gnuradio:main Dec 2, 2023
12 of 14 checks passed
@willcode willcode added ported-to-3.10 Has been ported to 3.10 and removed Backport-3.10 Should be backported to 3.10 labels Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants