[SciPy-Dev] New function in scipy.signal.lti to generate Nyquist plot
Pierre Haessig
pierre.haessig@crans....
Fri Feb 8 08:16:55 CST 2013
Hi Niklas,
Le 08/02/2013 14:00, Niklas Kröger a écrit :
> I am new to the development of open source software, so please don't
> be too harsh if I make/made any major mistakes.
> I recently forked the scipy main branch on github and added a function
> in the "signal" submodule. I have been using it during this semester
> in one of my classes and I missed the ability to generate a Nyquist
> plot, so I implemented it myself and figured others might find this
> useful too.
I think your PR is a valuable addition to scipy.signal. I've a few
comments though but I should just state first that I'm not a regular
user of scipy.signal so that my comments may not be entirely
appropriate. Main comment is about changing the name "nyquist".
I think there are two separate things in your PR: one is the ability to
evaluate the frequency response over some frequency grid. A separate
this is plotting a Nyquist diagram, i.e. plotting this frequency
response in the complex plane. Plotting is not (and I guess should not)
be implemented in the function you propose.
Therefore, I think the function you propose (which is a great complement
to the preexisting bode function by the way) should be named more
something like "freqresp".
I just did a bit of homework in matlab documentation and that's how I
got this name "freqresp":
http://www.mathworks.fr/fr/help/control/ref/freqresp.html
Of course we could use a slight variation of this name, but I've no
better idea.
This being said, I like your docstring.
In particular, plotting a Nyquist diagram in the docstring a is very
nice and short usecase. Only I would change the first line
"""Calculate nyquist plot of a continuous-time system."""
to
"""Calculate the frequency response of a continuous-time system over a
grid"""
Finally, I think the default value for the grid size "n" should be set
to the same default as bode(..). I've no guidelines on how to choose
this number though. Bode is 100, you propose 10^4. What about 10^3 ;-) ?
best,
Pierre
PS : within your PR, you may correct a tiny preexisting docstring glitch
in _default_response_frequencies(A, n) (line 617). I think
n : int
The number of time samples to generate
should read instead
n : int
The number of *frequency* samples to generate
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.scipy.org/pipermail/scipy-dev/attachments/20130208/d2bc6b7c/attachment.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
Url : http://mail.scipy.org/pipermail/scipy-dev/attachments/20130208/d2bc6b7c/attachment.bin
More information about the SciPy-Dev
mailing list