neurodsp-tools / neurodsp

Digital signal processing for neural time series.
https://neurodsp-tools.github.io/
Apache License 2.0
284 stars 62 forks source link

Split general filter function into specialized functions #99

Closed srcole closed 5 years ago

srcole commented 6 years ago

We talked about how the current filter function is a bit of a mess to deal with because of the different requirements for bandpass, highpass, lowpass, bandstop.

After discussing, @TomDonoghue and @rdgao and I generally agreed that it would be cleaner if we split them up into different sub-functions that could alternatively be called in addition to the general function

srcole commented 6 years ago

I started working on this.

If we split this up into sub-functions and want it to have the same input checking and functionality as the master function, we end up with A LOT of duplicate code, which I think is worse than how it is now.

The alternative is to make this into private functions that are called by the master function. However, there are many different parts of the master function that check what the filter type is (for raising errors, for designing the filter, for determining transition band). Therefore, approach either requires multiple private functions per filter type or still some duplicate code. In the former case, the general function is not much cleaner than it is now. In the latter case, there's again a large amount of duplicate code.

After looking into this, I think we should just keep the code as it is now and delete the issue.

Thoughts @TomDonoghue and @rdgao ?

rdgao commented 6 years ago

just looked through filt.py again, and I would agree in principle. Most of the code actually just deals with input checking/parsing and computing transition bandwidth, the filtering itself is just L145-168, so it's unclear how to naturally break this up.

I think our original thought was to have separate functions for LP, HP, etc, but those functions will just be splitting up those 20 lines above, or repeat input checking in all of them.

I think one option to make this function less unwieldy is to make compute_transition_bandwidth a separate thing, but overall # of lines will not decrease. Another option is to have input checking as a separate function, since it's super long, but only to similar (aesthetics) effects. But I don't know enough about the python ethos to say whether that's moving in a positive direction.

But I think we can at least agree to untag for 1.0?

srcole commented 6 years ago

fair. Yeah, splitting up the function into these sub-functions would make it easier to read the code. Shouldn't result in API change, so yeah, I removed the 1.0 tag.

TomDonoghue commented 5 years ago

Merged in #132 . Closing.