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

[BUG] - Technical Audit: Fix numerical instabilities for IIR filters #198

Merged
merged 5 commits into from
Jun 27, 2020

Conversation

elybrand
Copy link
Contributor

@elybrand elybrand commented Jun 25, 2020

This PR addresses issue #193 for the filt submodule. Specifically, the following modifications have been made:

  1. Update function call in fir.py for firwin(). We now feed in the sampling rate directly, rather than feeding in the deprecated argument nyq, or the nyquist frequency.
  2. Previously a warning was raised if you asked for a IIR butterworth filter that was anything but a bandpass. That warning has now been removed.
  3. Most importantly, I made changes to how IIR filters are now computed. Instead of computing a and b coefficients of the transfer function, we now compute the second order series expansion. This addresses numerical instabilities for butterworth filters of higher orders. I did not want to change any of the checks or utils for FIR filters, so I created new functions for IIR filters by prepending "sos_" to the function handles. I updated doc strings in all of the functions which referenced a and b values for IIR filters. Finally, I updated the test test_apply_iir_filter to take as input a second order series expansion rather than two arrays of a and b coefficients. TL;DR: IIR filters and related functions now exclusively use second order series representations. FIR filters and related functions are left untouched. This addresses numerical instabilities.

@elybrand elybrand changed the title Issue/filtr [BUG] - Fix numerical instabilities for IIR filters Jun 25, 2020
Copy link
Member

@ryanhammonds ryanhammonds left a comment

Choose a reason for hiding this comment

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

Hi @elybrand, I had minor comments about adding tests for the new sos funcs, and moving shared code between check_filter_properties and sos_check_filter_properties to a separate func so it's not duplicated. We'll also need to update the api.rst so the new sos funcs are rendered by sphinx for the neurodsp site. I can directly push these changes to this PR if you don't mind?

neurodsp/filt/checks.py Outdated Show resolved Hide resolved
neurodsp/filt/checks.py Outdated Show resolved Hide resolved
neurodsp/filt/checks.py Outdated Show resolved Hide resolved
@elybrand
Copy link
Contributor Author

@ryanhammonds I am more than okay if you'd like to push those changes to the PR directly.

@TomDonoghue
Copy link
Member

Thanks @ericlybrand for the updates here.

Of the updates, 1 & 2 look great! For 3, the update looks great for the technical fix, and I have direct pushed a refactor of how this gets organized into the code.

Tagging back in for review:

  • @elybrand : please have a look through the new code, and check that the way coefficients are now handled makes sense / looks good, and I that haven't mangled any of the technical aspects here.
  • @ryanhammonds : please review the code overall. I did not do any work to extend the tests to explicitly check & cover the new aspects of managing filter coefs - so please review overall, and then add a commit to add test coverage (I'm assuming we need something added here).

@TomDonoghue
Copy link
Member

Closing and re-opening to check a settings thing.

@TomDonoghue TomDonoghue reopened this Jun 26, 2020
@TomDonoghue TomDonoghue added 2.1 Updates to go into a 2.1.0 release algorithm A problem with how an algorithm / computation is designed or implemented. labels Jun 26, 2020
Copy link
Member

@ryanhammonds ryanhammonds left a comment

Choose a reason for hiding this comment

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

Everything looks good here! I have updated tests and increased coverage a bit.

Copy link
Contributor Author

@elybrand elybrand left a comment

Choose a reason for hiding this comment

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

Everything looks good to me.

@TomDonoghue
Copy link
Member

Alright, go team! This all looks good to me, so I'm merging in!

@TomDonoghue TomDonoghue merged commit d66bbee into neurodsp-tools:master Jun 27, 2020
@elybrand elybrand deleted the issue/filtr branch June 29, 2020 03:02
@TomDonoghue TomDonoghue changed the title [BUG] - Fix numerical instabilities for IIR filters [BUG] - Technical Audit: Fix numerical instabilities for IIR filters Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1 Updates to go into a 2.1.0 release algorithm A problem with how an algorithm / computation is designed or implemented.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants