Skip to content

Conversation

WarrenWeckesser
Copy link
Member

No description provided.

@rgommers
Copy link
Member

API questions:

  • Wouldn't it be better to create a more general freqz function that takes any filter representation?
  • Do you have to follow freqz with the badly named keywords and the out of place plot keyword?

@larsoner
Copy link
Member

Maybe this would be a good opportunity to make a more general function named frequency_response or so, that took any of the filter types and had sensibly named arguments (and no plot). Then freqz could be deprecated. Not sure about scipy's deprecation policy, though...?

@endolith
Copy link
Member

  1. agree about making a new function like freq_resp, where the first parameter is a complete system (tf, zpk, ss, sos, lti, ...), but would analog vs digital be separate functions, or a flag?
  2. agree that plot parameter is mostly useless and should be left out. it's better to just plot it as a separate command
  3. what's mpsig.py? will there be separate mpmath-enhanced versions of functions? There was discussion about an mpmath version of bessel and a legendre filter here: scipy.signal: Bessel filters with different normalizations #3763

@rgommers
Copy link
Member

freqz is pretty heavily used I suspect and not really buggy, so we should probably leave it alone. Only add to its docstring that use of the new function is preferred.

@WarrenWeckesser
Copy link
Member Author

I simply copied the API of freqz, but I have no love for that API, and a new, more general function is a good idea. I also agree that freqz should be left alone. It could be deprecated in the future (pre 1.0?) without actually being removed for a very long time (if ever).

Any proposals for the API of the new function? Currently, we have the following representations of a linear filter:

(b, a)     -- Transfer function (includes FIR filters by setting a=1)
(z, p, k)  -- Zeros, poles, gain
sos        -- Second order sections (array with shape (n, 6))

I could see adding a single input, single output state-space representation in the future: (A, B, C, D).

I have thought a little bit about this, and my only conclusion so far has been that I don't like the idea of distinguishing which format is being used by checking the length of the input argument (e.g. if the length is 2, it must be (b, a), if 3 it must be (z, p, k), etc.) That feels too implicit and brittle.

Instead of a new function, a more radical approach is to create a LinearFilter class, with, say class methods such as LinearFilter.fromsos(sos) for creating instances from existing representations, methods such as .tozpk() for obtaining the various representations of the filter, a .filter() method to apply the filter to an input (which could be used instead of lfilter or sosfilt) and with a freqresp() method. There would be overlap with the lti class (since a linear filter is a linear time-invariant system). In fact, maybe there is no need for a new class, if lti can be enhanced as needed, but I haven't thought too much about that. For example, I'm not sure how the analog vs. digital distinction should be handled.

@WarrenWeckesser
Copy link
Member Author

@endolith asked

what's mpsig.py?

That is only for testing, and the test that uses it is only run if mpmath or sympy.mpmath can be imported. I stole that approach from the tests in scipy.special (and there is room for some D.R.Y. clean up here).

@WarrenWeckesser WarrenWeckesser changed the title ENH: signal: Add the function sosfreqz. WIP: signal: Add the function sosfreqz. Jan 25, 2015
@larsoner
Copy link
Member

I think you mentioned having a LinearFilter class before, and it seems to be the cleanest solution for the issues with having multiple filter types. I have never used the LTI class, but at least by its naming it sound like a good place to piggyback onto if we can.

@endolith
Copy link
Member

I don't like the idea of distinguishing which format is being used by checking the length of the input argument (e.g. if the length is 2, it must be (b, a), if 3 it must be (z, p, k), etc.) That feels too implicit and brittle.

I think this is ok, as none of the existing filter formats has the same shape. There are already functions that do this, like freqresp, lti, impulse, bode, dimpulse, etc.

dimpulse requires you to combine the system coefficients with a sampling frequency, though, like (zeros, poles, gain, dt), so you can't just create a tuple object like zpk = butter(..., output='zpk') and then call dimpulse(zpk, ...). The filter design functions work with normalized frequency so the digital filter representations don't care about sampling rate, but it would be a good option for a frequency response function. Something like this?

 scipy.signal.freqrespz(system, worN=None, whole=0, fs=None)

freqz has worN while freqresp has separate w and n, but both can't be specified at the same time, so I think it makes sense to combine them?

I do support creating a filter object class, though, that stores the coefficients in whichever way it was initiated and converts to the others when necessary.

@rgommers rgommers removed this from the 0.16.0 milestone Apr 4, 2015
@rgommers
Copy link
Member

rgommers commented Apr 4, 2015

@WarrenWeckesser I removed the 0.16.0 Milestone, because it looks like this still needs significant changes and isn't really required for 0.16.0

@WarrenWeckesser
Copy link
Member Author

That's fine, Ralf. Thanks for getting the 0.16 release going.

@pv pv mentioned this pull request Aug 11, 2015
@cbrnr
Copy link
Contributor

cbrnr commented Aug 11, 2015

I think this function provides really important functionality - it would be great if sosfreqz could go into the next release for a quick solution.

@rgommers
Copy link
Member

We now have proper classes (StateSpace, ZerosPolesGain and TransferFunction) for the different formats, so a better API is probably easier to design now.

@pv pv added the needs-work Items that are pending response from the author label Feb 16, 2016
@WarrenWeckesser
Copy link
Member Author

I don't think I'll have time to get back to this for the 0.18 release, so if anyone want to take over this PR, or take whatever looks useful and start a new pull request, that would be fine with me.

@larsoner
Copy link
Member

larsoner commented Jun 7, 2016

If we're okay with having sosfreqz then I could clean it up for merge.

+1 from me on using sosfreqz instead of waiting until we solve the big API issues (e.g., #6137). I think we're still kicking the can down the road on how to unify these functions, and it seems wasteful to have PRs like this one languishing for almost a year and a half without merge.

I'd actually also like to write e.g. sosfiltfilt, but I doubt there is time for 0.18, since it would help complete the function options.

@e-q
Copy link
Contributor

e-q commented Jun 7, 2016

I'm all for it being included as its own function for now, I'm happy to help/review for some quick turnaround. I'd also like to eventually see some SOS equivalents of functions like filtfilt and lfiltic, not to mention LTI objects...

Over in #6059, I have tentatively named a new response function freqs_zpk, which would imply something like freqz_sos here, but I don't really care how we name them; let's just be consistent between the two PRs.

@larsoner
Copy link
Member

Closing for #6328

@larsoner larsoner closed this Jun 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement needs-work Items that are pending response from the author scipy.signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants