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

Improve iir filters docs #221

Merged
merged 2 commits into from
Mar 26, 2024
Merged

Conversation

Jalmenara
Copy link
Contributor

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 dancazarin changed the base branch from main to dev March 26, 2024 13:46
@dancazarin
Copy link
Member

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
Copy link
Contributor Author

Jalmenara commented Mar 26, 2024

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!

@dancazarin dancazarin merged commit f748564 into kfrlib:dev Mar 26, 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