Skip to content

BUG: Fix the automatic frequency selection of scipy.signal.bode. #432

Merged
merged 1 commit into from Mar 11, 2013

4 participants

@WarrenWeckesser

The automatic selection of the frequencies could miss zeros or resonance peaks, so use the existing function scipy.signal.findfreqs instead of _default_response_frequencies.

Specific changes:

  • Modifed the function scipy.signal.bode to use scipy.signal.freqs.
  • Added a docstring to scipy.signal.findfreqs.

More details

The function _default_response_frequencies in ltisys.py, used by signal.bode, attempts to compute a good range of frequencies for showing the frequency response of an LTI system. This actually duplicates the functionality provided by the function findfreqs in filter_design.py.

Because _default_response_frequencies doesn't use the zeros or the imaginary parts of the poles of the system, it can generate bad frequency ranges.

For example, the follow either fail or miss important features of the frequency response:

>>> w, mag, phase = bode(([1],[1, 0, 100]))    # Raises exception
>>> w, mag, phase = bode(([1],[1, 1e-2, 100])) # Misses the resonance peak near 10
>>> w, mag, phase = bode(([0.01j, -0.01j], [2,2], [0.5])) # Misses the zero at 0.01.

Compare the plots created by semilogx(w, mag) to the corresponding plots with data generated by scipy.signal.freqs, e.g

>>> system = lti([1], [1, 1e-2, 100])
>>> fw, fh = freqs(system.num, system.den, worN=1000)
>>> semilogx(fw, 20*numpy.log10(abs(fh)))

So in this pull request, I have changed the bode function to use scipy.signal.freqs instead of _default_response_frequencies. This improves the behavior of bode and removes redundant functionality.

I also added a docstring to findfreqs in filter_design.py. This is the function used by freqs to find the range of frequencies when the frequencies are not specified. It takes into account the zeros and the poles (real and imaginary parts) to choose the frequencies.

In the cases where _default_response_frequencies worked, it generally produced a larger range of values than findfreqs does, and this often looked better because it went a little further into the asymptotic ranges. Whether findfreqs should be tweaked to generate a wider range is something that could be discussed here or in another pull request.

@WarrenWeckesser WarrenWeckesser BUG: Fix the automatic frequency selection of scipy.signal.bode.
The automatic selection of the frequencies could miss zeros or resonance peaks, so use the existing function scipy.signal.findfreqs instead of _default_response_frequencies.

Specific changes:
* Modifed the function scipy.signal.bode to use scipy.signal.freqs.
* Added a docstring to scipy.signal.findfreqs.
aac6bd0
@bjornfor

You are right, _default_response_frequencies() has bugs. But I don't yet understand all of what findfreqs() does. So I'm currently trying to figure that out (and testing it and comparing with Matlab). And pondering whether adding clarifying comments to findfreqs() or fixing _default_response_frequencies() is the better solution.

Taking zeros into account in _default... is not difficult. It's when the poles/zeros are in the positive half-plane that I am unsure of what to do.

@WarrenWeckesser

We want a function that can automatically create a good range of frequencies for an LTI system, but we don't need two of them. findfreqs is already there and works pretty well, which is why this pull request drops _default_response_frequencies in favor of findfreqs. Rather than fix _default_response_frequencies, why not improve findfreqs? Note that improve might eventually mean completely replace the code. (But that would be done in a separate pull request.)

The code in findfreqs appears to use some numerical heuristics for finding a good frequency range. It also includes the flourish of rounding the ends of the interval to powers of 10. It is easy enough to read the code to know what it does, but unfortunately, the reasoning behind the heuristics is not explained in the code, so why it does it is a bit opaque. It also makes it tough to add tests. Updating findfreqs with well-documented code and tests would be a welcome contribution. (I don't think this pull request has to wait for such a change, though.)

@bjornfor

Good points. Ok, you get an ACK from me. Thanks for fixing bugs in bode() :-)

I'll add "document/refactor findfreqs" to my TODO. But I wouldn't mind if someone beat me to it :-)

@pv
SciPy member
pv commented Mar 11, 2013

I guess if both of you agree, this can be merged?

@bjornfor

Yes, I agree. Let's merge this to fix bode and remove duplicated code.

@pv pv merged commit aac6bd0 into scipy:master Mar 11, 2013
@pv
SciPy member
pv commented Mar 11, 2013

Thanks, merged.

@hazelnusse

@pv @bjornfor @WarrenWeckesser I filed a bug on a previous version of ltisys.py:

http://projects.scipy.org/scipy/ticket/1863

It is still present with what was just merged into master. I believe it is

https://github.com/scipy/scipy/blob/master/scipy/signal/ltisys.py#L904

I haven't explored this completely, but I think the issue is when sys.num has dimension 2 rather than dimension 1. This happens, for example with the following bit of code:

import numpy as np
from scipy.signal import bode

# Basic double integrator system
A = np.array([[0, 1], [0, 0]])
B = np.array([[0], [1]])
C = np.array([[1, 0]])
D = np.array([[0]])

w, mag, phase = bode((A, B, C, D))
@hazelnusse

Unless I'm using bode incorrectly, the example I gave should probably be added as some sort of test to make sure the bode() function supports the (A,B,C,D) interface as advertised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.