search

kfrlib / kfr

Fast, modern C++ DSP framework, FFT, Sample Rate Conversion, FIR/IIR/Biquad Filters (SSE, AVX, AVX-512, ARM NEON)
https://www.kfrlib.com
GNU General Public License v2.0
1.62k stars 248 forks source link

Improve iir filters docs #221

Closed Jalmenara closed 3 months ago

Jalmenara commented 3 months ago

Hello @dancazarin ,

I am migrating a code from matlab to cpp and I am facing issues very similar to #148 and #191. Furthermore, I totally agree with #168 regarding the documentation; in particular, of IIR filters.

After some trial and error I'd like to document my findings regarding the input parameters of the iir_filter functions. Since I can choose where to do that, I think the best place possible is the original repo itself. Thus I have forked and opened this PR.

This is just a draft of where the improvements in the docs should go, as far as I understand. If the commits are correct, the same approach could be applied systematically to the rest of IIR filters.

In fact, I think it would be even better to make the functioon headers and parameters analogous to the FIR filters, to make it more intuitive. If in FIR input frequencies are normalized with respect to f_nyquist_Hz=f_sampling_Hz/2, shouldn't the same criterion be applied for IIR functions? If it is OK, we can discuss about it in this PR. I hope it was OK to open it directly with a couple of commits without opening an issue first.

dancazarin commented 3 months ago

Hello @Jalmenara,

The PR looks great, thank you for your contribution. It's perfectly ok to start with the PR here. Would you mind if I merge the PR now? The math behind filter design functions are meant to reflect python's numpy/scipy behavior (as mentioned in #168) and be numerically compatible with python as much as possible. Thus the difference with matlab.

Making filter API more consistent is definitely a way to go but a lot of projects depend on KFR and significant changes may affect them.

Jalmenara commented 3 months ago

Hello. Thanks for the quick reply!

Yes, sure, you can go ahead and merge. I'll probably identify other documentation improvements while I continue my implementation, but I can open a new, larger MR when the time comes.

Regarding the API, yes, I totally understand. Perhaps we could start by an overloading of the current iir_design methods to match the headers of scipy.signal.iirfilter...? I think that would keep retrocompatibility and improve the overall experience. For instance, in that method, by default, frequencies are normalized with respect to Nyquist (unless optional arg. fs= is given). I feel that is a more natural "default behavior" (which should at least exist)

Additionally, due to my matlab background, I feel that adding the scipy's "matlab-style-iir-filter-design" methods would also help a lot. What do you think? https://docs.scipy.org/doc/scipy/reference/signal.html#matlab-style-iir-filter-design

Thanks for your time!