New function in scipy.signal.lti to generate Nyquist plot
Hi Scipy-Dev team, 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. Eagerly I quickly coded something and created a pull request on github where I then learned, that the decision about adding new functionality is made on this mailing list. If you want to take a look at the code you can find my commits in this pull request: https://github.com/scipy/scipy/pull/418 Like I said, I am entirely new to all of this and just hope to provide something useful to this great project I enjoy using. Best regards, -Niklas Kröger.
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
Hi Pierre, I think you are right. I chose the name because of the nyquist function from octave, which I used before I had this python version coded. However since my function doesn't generate the plot but just the values, you are right. Regarding the grid size "n": I was unsure of that value and I just chose 10^4 based on the plots I generated with the function. For simple functions like the example this number is very high. However I ran into some functions which could not be plotted precisely because of too few generated values. I will append an example of what I mean (only showing the plot for w->+inf to make the plot less crowded). [image: Inline-Bild 1] [image: Inline-Bild 2] As you can see n=10^4 is still not enough to make a really nice and round plot. But in my opinion it's close enough to see the actual plot. Of course you could argue, that anyone who generates plots of such complex transfer functions, could just set the value of n up himself. For me it's not important what default n is set to, 10^4 was just the first value that seemed to fit my needs. Anyway, I will change the name of the function as you said and adjust the docstring. Best regards, -Niklas.
participants (2)
-
Niklas Kröger -
Pierre Haessig