Discrete omega-damping plot and tweaks #193
Conversation
control/pzmap.py
Outdated
|
|
||
| __all__ = ['pzmap'] | ||
|
|
||
| def zgrid(plt): |
There was a problem hiding this comment.
The zgrid function needs a docstring + some comments in the code to describe what is going on.
There was a problem hiding this comment.
Ok! Do you have any example I could be based on?
There was a problem hiding this comment.
Take a look at nichols.py and, in particular, the nichols_grid() function.
control/pzmap.py
Outdated
|
|
||
| __all__ = ['pzmap'] | ||
|
|
||
| def zgrid(plt): | ||
| for zeta in linspace(0, 0.9,10): |
There was a problem hiding this comment.
The range for zeta should probably be something with a default set of parameters that can be reset by the user.
There was a problem hiding this comment.
making zetas and wns arguments so user can override default behaviour.
control/pzmap.py
Outdated
| @@ -40,11 +40,42 @@ | |||
| # | |||
| # $Id:pzmap.py 819 2009-05-29 21:28:07Z murray $ | |||
|
|
|||
| from numpy import real, imag | |||
| from .lti import LTI | |||
| from numpy import real, imag, linspace, pi, exp, cos, sin, sqrt | |||
There was a problem hiding this comment.
Rather than using numpy.pi, please use math.pi to avoid problems with sphinx. See commit aa6e0df
control/pzmap.py
Outdated
| an_i = int(len(xret)/2.5) | ||
| an_x = xret[an_i] | ||
| an_y = yret[an_i] | ||
| plt.annotate(str(zeta), xy=(an_x, an_y), xytext=(an_x, an_y), size=7) |
There was a problem hiding this comment.
Can/should we use parameters for the size? 7 seems like a magic number.
control/pzmap.py
Outdated
| @@ -75,16 +106,20 @@ def pzmap(sys, Plot=True, title='Pole Zero Map'): | |||
|
|
|||
| if (Plot): | |||
| import matplotlib.pyplot as plt | |||
|
|
|||
| if isdtime(sys): | |||
There was a problem hiding this comment.
This needs to be isdtime(sys, strict=True), otherwise it will be true for systems with unspecified timebases (which could be continuous or discrete).
There was a problem hiding this comment.
So we are treating systems with unspecified timebases as continuous (showing s-plane omega-damping - see next comment)?
There was a problem hiding this comment.
The idea is that if the timebase is unspecified, it can represent either a continuous-time or discrete-time system (since for many operations it doesn't matter). Because of this, if you call isdtime on a system for a system in which timebase == None, isdtime will return true. If you want something to only happen when a system is really a discrete time system, use isdtime(sys, strict=True).
control/pzmap.py
Outdated
| @@ -75,16 +106,20 @@ def pzmap(sys, Plot=True, title='Pole Zero Map'): | |||
|
|
|||
| if (Plot): | |||
| import matplotlib.pyplot as plt | |||
|
|
|||
| if isdtime(sys): | |||
| zgrid(plt) | |||
There was a problem hiding this comment.
I think we should make plotting the zgrid an option, rather than the default. This will give consistency with past behavior.
There was a problem hiding this comment.
I am actually having it similar to what we have on rlocus function, with an argument grid=False (even though IMHO plot is way better with the omega-damping grid)
I also realized we should also be using this on rlocus function, so I am making zgrid available for rlocus and making _sgrid_func() from rlocus available for pzmap.
There was a problem hiding this comment.
Yes, this sounds like a good approach. And if we can reuse an existing function, that is even better (although I note that _sgrid_func() is not all that well commented -:).
…zgrid to new python module.
|
Done some major refactoring on sgrid function based on matplotlib projections. It is not perfect though, as there is not much documentation available on using it, but it works well when moving the plot around and zooming. Please take a look at it in pzmap and rlocus plots and let me know what you think. Currently the zgrid does not use projections so there is no update on zooming. |
|
why is the #3d0bf03 commit not merged yet ? I had to manually change pzmap.py file on my machine to get the zeros to be plotted |
|
I am new to this repo, so forgive me this question: Why does this PR have so many commits? As this PR seems to address plotting enhancements, wouldn't it be cleaner (and easier to track) to do a rebase and squash all commits into a single "plotting enhancements" commit? |
|
@jwdinius just because this is my first contribution to an open-source project and no one suggested it so far :) Should I do this? I am actually waiting for a word from python-control maintainers |
|
Either way is OK. 5 commits is not that many. @Sup3rGeo: go ahead and squash if you want to give it a shot. @Abubakr95: I'm still waiting to find some time to review the changes and merge. |
|
gentle ping |
Other tweaks: