-
Notifications
You must be signed in to change notification settings - Fork 65
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
Conversation
…efficients. Update test to take sos coefficients.
There was a problem hiding this 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?
@ryanhammonds I am more than okay if you'd like to push those changes to the PR directly. |
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:
|
Closing and re-opening to check a settings thing. |
There was a problem hiding this 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.
There was a problem hiding this 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.
Alright, go team! This all looks good to me, so I'm merging in! |
This PR addresses issue #193 for the filt submodule. Specifically, the following modifications have been made: