Charles R Harris <charlesr.harris <at> gmail.com> writes:

On Sun, May 10, 2009 at 8:41 PM, Tom K. <tpk <at> kraussfamily.org> wrote: (I sent this to scipy-dev <at> scipy.org, thinking it would appear here, I am horribly confused - sorry for the duplication SIMPLE REVIEW NEEDED - SINGLE FUNCTION Link to Trac ticket: http://projects.scipy.org/scipy/ticket/902 This is adding a highpass, bandpass, bandstop, and multiband capability to signal.firwin. I submitted a patch attached to the ticket to filter_design.py. I also added a test class in tests/test_filter_design.py. Let me know if there are any problems with the patch as this is my first patch to scipy and I am new to SVN.

I think class BandPass is too general a name for a method based on windowing. Have you looked at the remez filter design? You can design the type of filters you are talking about using it also. Chuck

For PassBand, which of these options sounds best: 1) rename to _PassBand, or FirwinPassBand 2) make local to firwin function 3) remove class altogether and just go with a list of (left, right) tuples I hadn't seen remez as the wrapper is in signaltools, not filter_design, but thanks for the pointer. See this thread http://www.nabble.com/firwin-upgrades-td23246480.html for discussion about the interface to firwin. The question I have, what is the right way to provide a convenient shorthand for describing a multiband filter with just a vector? The Signal Processing Toolbox has this divergence which maybe we want to avoid. E.g. fir1 takes a list of band edges and you leave 0 and 1 off (they are present implicitly) and the magnitudes - which are not required to be specified by the user - alternate between 1 and 0; the stop, high, and DC-0 options change the magnitude to start at 0 instead (whereas pass, low, DC-1 start with the default magnitude of 1). Remez and firls take only band edges in pairs and require the same for the magnitude - this allows for a piece-wise linear desired function with unspecified regions in between. I haven't looked at the newly submitted fir2 (which is sitting in a trac issue) but the Signal Processing Toolbox has a list of edges and magnitudes with 0 and 1 implicit again IIRC. fir2 doesn't require paired bands, you just specify a piecewise linear function and that gets interpolated over the entire band and inverted to the impulse domain. The more I think about it, the functions seem slightly different in the ways that the algorithms work and so it makes some sense that they each accept arguments that are tailored to be the most natural for that algorithm. It looks like remez in scipy takes only constant bands. Hence it may make sense to unify it with firwin. firwin would then allow varying amplitudes (the feature I added has them all the same). The cost is some simplicity, but perhaps some "smarts" could be added that make the magnitude vector implicit if unspecified (which could work for remez in the same way). Is that what you would propose?

Thanks for the earlier review and discussion Chuck and others. Sorry it has taken so long to get back to this issue. For now I have moved the PassBand class inside firwin to take it out of the filter_design module's local variables; let me know if you'd prefer a different option for this class. I've submitted a new patch to the track issue, http://projects.scipy.org/scipy/ticket/902 I think this patch is worth including for the following reasons: - it adds functionality to firwin that has been requested by scipy users, in a way that is backwards compatible with the previous lowpass-only functionality, that functionality being highpass, bandpass, bandstop, and general multiband filters; also an option to *not* scale the filter is added - it adds a *test* for firwin where none existed before, testing different filter lengths and options and making sure the frequency response is sane; the test of course exercises not only the old functionality but the new as well. Other questions about organization of signal (e.g. creating new modules for fir filtering, or equiripple and window fir filter submodules) and harmonization of different filter design routines (remez and window) are not precluded and are largely orthogonal to incorporation of the new functionality to this one small routine. I see scipy as being an incredible resource and I would like to add this tick mark to its feature list to compete with other commercial tools. I would love to have scipy developed into a world class tool in every way so that potential adopters are not able to cite the lack of some particular feature as an excuse for sticking with a different commercial tool.