-
Notifications
You must be signed in to change notification settings - Fork 113
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
Expose resample_filter
optional arguments in resample
, FIRFilter
#621
base: master
Are you sure you want to change the base?
Conversation
in resample
_resample_filter
remove inbounds, correct typo
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #621 +/- ##
==========================================
+ Coverage 97.97% 98.12% +0.15%
==========================================
Files 19 19
Lines 3254 3258 +4
==========================================
+ Hits 3188 3197 +9
+ Misses 66 61 -5 ☔ View full report in Codecov by Sentry. |
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.
Mostly LGTM, but if the missing coverage reports are legitimate, some more tests are in order.
Co-authored-by: Martin Holters <[email protected]>
How do you feel about bumping the version to 0.8.1 as part of this PR and tagging a new release once it's in? |
Sure, why not. Also, do you think |
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.
Ooof, +343 -267 lines for a relatively simple change due to code reorganization and tangential changes. That really makes code review unnecessarily hard. I'll try to find time/motivation for a closer look at this, but if you get impatient, you could help by moving NFC changes like the introduction of reference data
or the like to separate PRs.
5d925ca
to
0148012
Compare
Just passes on arguments in the most obvious manner.
Additionally, make some
FIRKernel
fields const, and removed unnecessary@inbounds
, and added restrictions topad_length
infiltfilt
to prevent oob.Also wrote some minimal documentation for
resample_filter
. Need some help here, I just guessed the meaning ofrel_bw
...Should help with #620