Fix chebyshev band pass and band stop returning all NaN - #849
Fix chebyshev band pass and band stop returning all NaN#849adityasingh2400 wants to merge 3 commits into
Conversation
The chebyshev band pass and band stop designs in DSPFilters derive from ChebyshevI::Design::TypeII, which takes five params in the order sample rate, order, center frequency, band width, ripple. The low pass and high pass designs derive from TypeI and take four, with the ripple in slot three. perform_bandpass and perform_bandstop wrote the ripple into slot three for every chebyshev filter type. For band pass and band stop that overwrote the band width and left the real ripple slot unwritten, so the design was built from a bogus band width and an uninitialized ripple and every output sample came back NaN. The exit code was still STATUS_OK. Write the ripple into slot four for the band designs and clear the params struct first, since Dsp::Params has no constructor.
|
The parameter-slot correction matches the bundled DSPFilters band-design parameter order, and Please add a focused regression case exercising both |
|
Sorry, that was codex and autoreview, change looks fine, thanks |
Drives both filters with an in-band and an out-of-band tone, asserts every output sample is finite, and checks the kept tone survives above 0.8 while the rejected one falls below 0.05.
|
Added It drives a 10 Hz plus 50 Hz signal through both On master it fails at the first assertion with |
|
I missed your follow-up before posting that, sorry. I had started on the tests when the first comment came in and did not re-check the thread before pushing. The test and the CI line are a separate commit, so if you would rather merge this as the one-line fix you already reviewed, say so and I will drop it. If you want to keep it, nothing further is needed. |
DataFilter.perform_bandpassandDataFilter.perform_bandstopreturn an array that is entirely NaN whenever the filter type isCHEBYSHEV_TYPE_1orCHEBYSHEV_TYPE_1_ZERO_PHASE. The exit code is stillSTATUS_OK, so nothing is raised and the NaN silently poisons whatever runs next, band powers, PSDs, the ML metrics. Butterworth and Bessel are fine, and Chebyshev low pass and high pass are fine, so only these two entry points are affected.The cause is a parameter slot mismatch against the bundled DSPFilters library.
ChebyshevI::Design::LowPassandHighPassare built onTypeI, which takes four params in the order sample rate, order, cutoff, rippleDb, so the ripple really does belong in slot three there.ChebyshevI::Design::BandPassandBandStopare built onTypeII, which takes five, sample rate, order, center frequency, band width, rippleDb, as declared bygetParamInfo_3 = defaultBandwidthHzParamandgetParamInfo_4 = defaultRippleDbParaminthird_party/DSPFilters/include/DspFilters/ChebyshevI.h. Bothperform_bandpassandperform_bandstopwroteparams[3] = ripplefor every Chebyshev type. On the band designs that overwrote the band width with the ripple value and left slot four unwritten, and sinceDsp::Paramsis a plain struct with no constructor andclearis never called, the design was computed from a bogus band width and an indeterminate ripple.The fix writes the ripple into
params[4]for the band designs, leaves the band width inparams[3], and callsparams.clear ()first so an unwritten slot can never be read as garbage again. Chebyshev low pass and high pass are untouched. This is why the bug survived so long: every Chebyshev call site in the repository,python_package/examples/tests/signal_filtering.py,cpp_package/examples/signal_processing/src/signal_filtering.cppandjulia_package/brainflow/test/signal_filtering.jl, uses low pass or high pass only. No example or test ever calls Chebyshev band pass or band stop.I built on macOS arm64 with
cmake .. && make -j8and drove the Python binding against the freshly builtlibDataHandler. Before the change, a 4096 sample signal at 250 Hz throughperform_bandpass(..., CHEBYSHEV_TYPE_1, 1.0)came back 4096 of 4096 NaN, and the same forperform_bandstop, while Butterworth and Bessel were clean. After the change, using a sum of sine probes at 1, 10, 25, 60 and 90 Hz, a 5 to 30 Hz Chebyshev band pass gives per tone gains of 0.0019, 0.9475, 0.8948, 0.0049 and 0.0002, closely tracking the Butterworth reference of 0.0012, 0.9991, 0.9544, 0.0176 and 0.0007. A 20 to 30 Hz band stop notches the 25 Hz tone to 0.0005 and leaves the others above 0.89.Two further checks confirm the specific slots. The band width is now honoured rather than being replaced by the ripple value: sweeping the band as 5 to 10, 5 to 30 and 5 to 60 Hz moves the 25 Hz gain from 0.0012 to 0.8948 to 0.9272 and the 60 Hz gain from 0.0003 to 0.0049 to 0.8890. The ripple now reaches the design and behaves monotonically: at 0.1, 1.0 and 3.0 dB the 10 Hz passband gain is 0.9943, 0.9475 and 0.8361, which is the expected deepening of passband ripple. The zero phase variant is also NaN free and correct.
For regressions I ran the synthetic board examples CI already runs,
denoising.py,signal_filtering.py,transforms.py,downsampling.py,band_power.py,band_power_all.py,windowing.py,ica.pyandcsp.py, and all pass.clang-formatwith the repository.clang-formatreports no change anywhere in the file. There is no assertion based test surface for the signal processing functions, the existing coverage is demo scripts that plot rather than assert, so I could not extend one. I am happy to add a Chebyshev band pass case to the examples if you would like the path covered in CI.