- Notifications
You must be signed in to change notification settings - Fork441
Nyquist plot improvements#534
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 6, 2021 • 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.
control/freqplot.py Outdated
| # If argument was a singleton, turn it into a list | ||
| if not hasattr(syslist, '__iter__'): | ||
| isscalar = not hasattr(syslist, '__iter__') |
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.
IsScalar sounds like it could be referring to the system. ‘Is_Single_system’?
control/matlab/wrappers.py Outdated
| from ..freqplot import nyquist_plot | ||
| # If first argument is a list, assume python-control calling format | ||
| if (getattr(args[0], '__iter__', False)): |
sawyerbfullerFeb 7, 2021 • 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 have seen this style elsewhere in the library so I am curious: isn’t hasattr(args[0], ‘iter’) both more readable and compact? (I can’t do backticks from my phone so underscores appear as bold)
bnavigatorFeb 7, 2021 • 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.
hasattr also returns True ifargs[0] is an iterable sequence, but empty. This code only branches ifargs[0] is iterable and the sequence does not cast to False (is empty).
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 see, thanks!
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.
getattr([], '__iter__', False) returns <method-wrapper '__iter__' of list object .... So looks like both of them would follow the branch?
Very nice! Plots look great and I think the new return values make sense. |
control/tests/nyquist_test.py Outdated
| @pytest.fixture(scope="function") | ||
| def figure_cleanup(): | ||
| plt.close('all') | ||
| yield | ||
| plt.close('all') |
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.
Did you try to use the existingmplcleanup (automatically loaded fromconftest.py)?
python-control/control/tests/conftest.py
Lines 111 to 124 ind75c395
| @pytest.fixture(scope="function") | |
| defmplcleanup(): | |
| """Workaround for python2 | |
| python 2 does not like to mix the original mpl decorator with pytest | |
| fixtures. So we roll our own. | |
| """ | |
| save=mpl.units.registry.copy() | |
| try: | |
| yield | |
| finally: | |
| mpl.units.registry.clear() | |
| mpl.units.registry.update(save) | |
| mpl.pyplot.close("all") |
Now that we don't test python2 anymore, we could directly map it tomatplotlib.testing.decorators.cleanup
Hmm, I wonder why the nichols_test xlim warning is back here. The test is runbefore nyquist test and already uses the mplcleanup fixture. |
* regularize argument parsing by using _parse_freqplot_args* fix bug in exception handling (ControlArgument not defined)* change printed warning to use warnings.warn* add unit tests for exceptions, warnings
* start tracing from omega = 0* add support for indenting around imaginary poles* change return value to number of encirclements (+ contour, if desired)* update MATLAB interface to retain legacy format* new unit tests
This PR provides improvements to Nyquist plots:
nyquist_plotcommand now returns the number of encirclements of the -1 point and (optionally) the contour used to generate the plot instead of the magnitude, phase, and frequency values. (The system response can be generated from the contour by simply evaluating the LTI system along the contour).indent_radius) and the direction of the indentation for the case of poles exactly on the imaginary axis (setindent_directionto 'left' or 'right'). Indentation can be turned off by settingindent_directiontonone).FancyArrowPatch, which provides a fixed size and aspect ratio in display coordinates (so zooming in doesn't not change the size and shape of arrows). The default arrow style can be overridden using the optionarrow_sizeparameter or thearrow_styleparameters.arrowskeyword, which allows you to specify the number of arrows or the specific locations (via a list of relative positions on the primary segment).mirror_styleparameter and defaults to a dashed line. The mirror image segment can be omitted by settingmirror_style=False.nyquistretains its original call signature and return values.Before:
After:
Other notes:
mirror_styleparameter will conflict with themirrorparameter in PRDescribing functions #521 (describing functions). The functionality is basically the same, but there will be a merge conflict in whichever gets merged second. (I think we should usemirror_style).config.defaultswith module namenyquist.control/tests/nyquist_test.pyunit test file serves as a pretty complete unit test, but can also be run in python/ipython to generate plots that you can see (and make sure they look OK).bodeinto a separate function (_parse_freqplot_args) that is used for bothbodeandnyquist.