- Notifications
You must be signed in to change notification settings - Fork441
Discrete omega-damping plot and tweaks#193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedFeb 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I haven't fully checked the functionality of the PR, but there are a couple of things that should be fixed before merging.
control/pzmap.py Outdated
| __all__ = ['pzmap'] | ||
| def zgrid(plt): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The zgrid function needs a docstring + some comments in the code to describe what is going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok! Do you have any example I could be based on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Take a look atnichols.py and, in particular, thenichols_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
makingzetas andwns 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Rather than using numpy.pi, please use math.pi to avoid problems with sphinx. See commitaa6e0df
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This needs to beisdtime(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 callisdtime on a system for a system in whichtimebase == None,isdtime will return true. If you want something to only happen when a system isreally a discrete time system, useisdtime(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. |
abouatef commentedApr 25, 2018
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 |
jwdinius commentedMay 13, 2018
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 notthat 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: